Stefan wrote: "a few comments on your changes [personal thoughts]"
My responses to your comments are below, inline. Stefan wrote: "I know you may have pointed out why you did that, but why would you do that? What happens then? The mouse pointer changes?" The cancelGesture method allows the CursorTool to perform any clean-up it needs. For example: A CursorTool implementation may need to close a file it has opened, or it may need to modify values in a Blackboard object. If you replace it with a dummy cursor tool, as we do now, without calling cancelGesture, you do not give a chance for this clean-up to take place. After the call to cancelGesture the TaskFrame closes. (I want to add a call to the CursorTool.cancelGesture method in the listener that responds to TaskFrame close events.) Stefan wrote: "What was the reason for this change? to avoid the use of the component Factory? Here I would leave the factory as I would assume that there is a reason why they used that. To remove a method sounds dangerous to me (usually one starts with introducing a depricated label.. but as it is your personel edition)" I belive the ComponentFactory class and the TaskFrame.setTask method were added by Paul Austing so that he could try out his Docking Windows PlugIn. While I don't have a problem with this change, I don't think we should leave it in the core permanently. We should replace it with a full factory system if we are going to support custom TaskFrames installed via plug-ins. If we aren't going to do this then we should remove the setTask method. The only time you add a Task to a TaskFrame is in the constructor. If you look at the method implementation it throws a runtime exception if you set the Task on a TaskFrame that is already set. I don't think it is a good idea to keep a method like this with a public access signature. Someone will think you can replace the Task on a TaskFrame when they see it, and you can't use the method to do that. Stefan wrote: "this makes things messy in a repository in terms of tracing and comparing old and new code. so I would avoid that in general for released code." OK. I'll keep this type of reorganization in my own fork. Two me the readability of the code is more important. Stefan wrote: "not sure about this. It sounds logic from my side. And as it is hidden it would not break any access method. So from my "non-programmers" perspective I would go with that change." This is a purely comsetic change as well. I did it to improve the readability of the code. Based on your other statements, I think we should leave this out of the core. Then, at least, we are consistent. Stefan wrote: "Adding something is ok. But you should outline why do you add this?" This gives a core or plug-in porgammer the ability to determine if a TaskFrame is a clone of another TaskFrame. There may be things you would want to do to the original TaskFrame that you wouldn't want to do to a clone. Stefan wrote: "if you want and can write tests then go ahead with this (However, for the stuff I programm, i.e. data analysis, I feel it is almost impossible to use unit tests... but I have also never programmed them)" OpenJUMP can be difficult to unit test. You run into problems when the object you want to test depends on other parts of the application that also depend on other parts of the application that also depend on other parts of the applicaiton...you get the idea. The ability to test the class under design without a bunch of set up work is called "testing in isolation". Some of this can be solved with mocks and stubs, but it is not easy. You have to be careful that you don't spend time testing your tests. It gets really ugly when you try to unit test code that is part of, or interacts with, OpenJUMP's gui. Still, I've become "test infected" and just about won't write code without a unit test. I find it saves me a lot of debuggin later, and is more efficient. Landon On Thu, Jul 10, 2008 at 9:33 PM, Stefan Steiniger <[EMAIL PROTECTED]> wrote: > Hei Landon, > > a few comments on your changes [personal thoughts] > >> However, I think there is a reasonable solution: >> >> I'll keep all of my changes in my own fork. I'll regularly present >> these changes to the OpenJUMP programming community. I'll clearly >> identify those changes that I think are necessary to avoid bugs to to >> enable a critical new function in the program. > this is good > >> This type of change >> will be distinguished from changes which are cosmetic or very minor. >> Then we can let the changes be approved and adopted by the group on >> there own merit. (I'll try not to get my feelings hurt when something >> doesn't get in.) :] > that is better :) > >> >> Let's look at my TaskFrame/Workbench Context changes as an example: >> >> Critical Changes: >> >> Call CursorTool.cancelGesture method when closing a TaskFrame. > I know you may have pointed out why you did that, but why would you do > that? What happens then? The mouse pointer changes? >> Remove TaskFrame.setTask method and modify >> WorkbenchContext.addTaskFrame method to no longer use >> ComponentFactory. > What was the reason for this change? to avoid the use of the component > Factory? Here I would leave the factory as I would assume that there is > a reason why they used that. > To remove a method sounds dangerous to me (usually one starts with > introducing a depricated label.. but as it is your personel edition) >> >> Cosmetic Changes: >> >> Group all public, protected, and private methods together in class file. > this makes things messy in a repository in terms of tracing and > comparing old and new code. so I would avoid that in general for > released code. >> Implement InternalFrameListener instead of using hidden >> InternalFrameAdapter class. > not sure about this. It sounds logic from my side. And as it is hidden > it would not break any access method. So from my "non-programmers" > perspective I would go with that change. >> Add isAClone member variable, setIsAClone public method. > Adding something is ok. But you should outline why do you add this? > >> Add Javadoc comments for all public methods. > this is always! appreciated :) > > >> Would we be interested in any of the cosmetic changes? > my 2 cents: in general no cosemtic changes on old code if it is not > necessary to make things work or fix a bug. The only exception to is > javadoc. We need this as there are (unfortunately) only a few wizards in > our group (Paul, Martin, Andreas, ...). > >> >> I want to point out that if we take a more assertive stance on unit >> testing JUMP cosmetic changes to the core would be less of a >> risk...Maybe I'll start working on some unit tasks for key classes in >> the core. > if you want and can write tests then go ahead with this (However, for > the stuff I programm, i.e. data analysis, I feel it is almost impossible > to use unit tests... but I have also never programmed them) > > > ------------------------------------------------------------------------- > Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW! > Studies have shown that voting for your favorite open source project, > along with a healthy diet, reduces your potential for chronic lameness > and boredom. Vote Now at http://www.sourceforge.net/community/cca08 > _______________________________________________ > Jump-pilot-devel mailing list > Jump-pilot-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/jump-pilot-devel > ------------------------------------------------------------------------- Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW! Studies have shown that voting for your favorite open source project, along with a healthy diet, reduces your potential for chronic lameness and boredom. Vote Now at http://www.sourceforge.net/community/cca08 _______________________________________________ Jump-pilot-devel mailing list Jump-pilot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/jump-pilot-devel
