Chakakhan Posted June 22, 2011 Report Share Posted June 22, 2011 This is a question to the dev leaders and probably of general interest to all new developers. I just finished work on a task and had a few questions about how I completed the it.In this case, I added some code to CInput to handle word delete keystrokes - this was the scope of the task request. Widget code is tricky stuff, so since I was already in there and I had added the logic for discovering word boundaries and was familiar with the code, I also added the code to handle moving around a word at a time. And since I was already in there and doing word stuff, I added the code to handle word selection via. mouse double clicks. Then I noticed that there was code for paste, but no code for copy and cut, so I added them as well. When I tested the cut/paste/copy stuff I discovered a really hard to find bug that made intra-application clipboard operations on Windows fail. So I fixed that bug.My question is, given the above scenario, how would you prefer me to handle such situations? Would you prefer that I would have stopped at some point and created new tasks? If so where?I did finally close the task (well changed it to review) and created a couple of new tasks for CInput because the patch was getting fairly large. Please let me know your preferences on such things.Cheers! Quote Link to comment Share on other sites More sharing options...
janwas Posted June 23, 2011 Report Share Posted June 23, 2011 Great job on that ticket! I have just reviewed, cleaned up and committed the patch.These features are very useful; thanks for adding them!I think your approach of adding similar features and fixing a bug that came up makes sense. There is no benefit to generating extra "paperwork" if the other tasks are related and you are willing and able to accomplish them in one sitting. I assume you have a local todo-list that provides a little morale boost as each entry is marked done, so there's no need to use trac tickets for that.However, the two extra tickets you created make sense, since they relate to different topics and might take longer to fix.As to patch size - we've had huge ones before; if that's called for by the size/scope of the task, so be it. It's good keep each patch related to a single issue, though, so we don't accidentally throw the baby out with the bathwater and/or get things confused Quote Link to comment Share on other sites More sharing options...
Chakakhan Posted June 23, 2011 Author Report Share Posted June 23, 2011 Ok Jan, thanks for the feedback. Sounds like we are in sync.I noticed you found a window handle that worked with the clipboard, glad to see that! Also, I noticed how you cleaned up the error handling in wclipboard. A lot of the older code is coded similar to the way wclipboard was (little or no error checking returning nums or zero instead of symbolic constants), so I will follow your lead and make similar changes when I run into that type of code.Cheers Quote Link to comment Share on other sites More sharing options...
janwas Posted June 23, 2011 Report Share Posted June 23, 2011 Oh yeah. Some of the code is quite old (mentioning problems with Win98 and 2000), and some of it was also designed to minimize size (being part of 4K demos).That's great, similar cleanups are very welcome Additionally, two minor requests: please use tabs (not spaces) and try to match the coding convention at that particular spot. Certain parts use if (expression), others write if(). It's nice to keep that consistent. Quote Link to comment Share on other sites More sharing options...
Chakakhan Posted June 23, 2011 Author Report Share Posted June 23, 2011 (edited) Additionally ... please use tabs (not spaces) ...Hi Jan, I thought I had keep tabs selected in VS2010, I will verify that before I make any more changes.CheersHi Jan, I verified that I have the use tabs turned on. I usually use spaces, so its possible I'm used to typing in spaces and that is where they originated . Anyway, I will be more careful about that now. Edited June 24, 2011 by Chakakhan Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.