Community
Participate
Working Groups
It looks like TaskRepository can't be extended or have some additional settings. Perhaps concrete implementation of the AbstractRepositorySettingsPage should return instance of the TaskRepository (or its subclass)
Created attachment 44076 [details] Proposed changes Here is the proposed changes
Created attachment 44077 [details] mylar/context/zip Task context for proposed refactorings
Will look at this after 0.5.3 goes out (scheduled for today).
Mik, do you have time to adress this any time soon? Thanks
Changes look good and patch applied. However, once this is further along we'll need to figure out if we want to promote inheritance for TaskRepository (e.g. making AbstractTaskRepository with an IssueTrackerRepository subtype), or have a final, customizable, and probably immutable class for TaskRepository. On another note, I worry that the single-sanbox plug-in will be too restrictive for you and others. I'm thinking of making an org.eclipse.mylar/sandbox folder in CVS that will contain seperate sandbox projects, can have an update site project, etc.
(In reply to comment #5) > Changes look good and patch applied. However, once this is further along we'll > need to figure out if we want to promote inheritance for TaskRepository (e.g. > making AbstractTaskRepository with an IssueTrackerRepository subtype), or have > a final, customizable, and probably immutable class for TaskRepository. Iither one is fine with me. Though it could be easier to handle persistence with generic customizable one (e.g. use something like map of properties) > On another note, I worry that the single-sanbox plug-in will be too restrictive > for you and others. I'm thinking of making an org.eclipse.mylar/sandbox folder > in CVS that will contain seperate sandbox projects, can have an update site > project, etc. Ops. It looks like I've included sanbox stuff into this patch. But it is probably good thing...
It appears that TaskRepositoryManager.saveRepositories() and readRepositories() can't handle any additional repository properties besides those hardcoded in TaskRepository.
I wonder if AbstractRepositoryConnector should provide String[] getRepositoryProperties() method that will return names of supported repository properties. Then TaskRepository will have map of those properties and allow RepositoryManager to deal with any custom attributes.
Created attachment 44464 [details] Changes for TaskRepository persistence and custom attributes I've also included changes for sandbox web provider.
Looks good and I do like this approach more than the subtype. But before I apply could you please add a unit test for a custom property to TaskRepositoryManagerTest?
Created attachment 44474 [details] Changes for TaskRepository persistence and custom attributes with test I've added a test case, however there are two existing tests are failing. org.eclipse.mylar.tasklist.tests.TaskRepositoryManagerTest testRepositoryPersistance(org.eclipse.mylar.tasklist.tests.TaskRepositoryManagerTest) junit.framework.AssertionFailedError: expected:<1> but was:<2> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:282) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:201) at junit.framework.Assert.assertEquals(Assert.java:207) at org.eclipse.mylar.tasklist.tests.TaskRepositoryManagerTest.testRepositoryPersistance(TaskRepositoryManagerTest.java:103) testRepositorySyncTimePersistance2(org.eclipse.mylar.tasklist.tests.TaskRepositoryManagerTest) junit.framework.AssertionFailedError: expected:<Wed Dec 31 19:00:00 EST 1969> but was:<Wed Jun 14 21:30:38 EDT 2006> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:282) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:71) at org.eclipse.mylar.tasklist.tests.TaskRepositoryManagerTest.testRepositorySyncTimePersistance2(TaskRepositoryManagerTest.java:195)
I tried to apply the patch, and if I run the suite without the patch all tests pass, with the patch those two tests fail. I launched a test workspace and this resulted in a fatal error causing previous tasks not to be associated with repositories (message is "Mylar: No repository associated with task"). So there is a bug with the patch. Btw, icons need to come as attachments since they won't apply as a patch.
Created attachment 44605 [details] /org.eclipse.mylar.sandbox/icons/obj16/web_repository.gif
Created attachment 44607 [details] Changes for TaskRepository persistence and custom attributes with test That should fix it.
Great. Patch applied. I'm going to request to move the sandbox into the org.eclipse.mylar/sandbox directory. I won't know exactly when it happens, but when it does you may need to take any additional patches that haven't been applied, save them locally, and then re-apply them to the new project. Will keep you posted.
Thanks Mik. By the way, I wonder if createTaskFromExistingKey() should be actually in AbstractAddExistingTaskWizard class, so it will at least stay close to its usage...
I have had this struggle . The bad thing about it is that it puts functionality that's not UI into the UI. The good thing about it is that it is where you expect it to be. Let's keep this in mind if we start doing another refactoring of the connectors.
Created attachment 44706 [details] EditRepositoryWizard fix for editing existing repositories This fix would allow to edit existing repositories. Otherwise custom properties are lost. This probably makes "edit" wizard the same as "new repository" wizard.
It is hard to let it go. Had to reopen again since saving custom properties didn't work with EditRepositoryWizard (purely an UI issue). Fix attached.
Yup, they're almost the same, but note that the edit one needs to refactor the task handles in performFinish(). MylarTaskListPlugin.getTaskListManager().refactorRepositoryUrl(oldUrl, newUrl); We could make the single wizard polymorphic to this... btw, this is one of the reasons I prefer to not put too much functionality into the wizards--sometimes it's perfectly fine to have duplication with UI things like wizards, and plugin.xml often forces this. But it's rarely OK to have that with the core functionality that we end up pushing into the managers and connectors. Patch applied.
Anything left to do here?
Not sure if it belong to this report. Here is the list of possible refactorings: -- AbstractRepositoryConnector.openEditQueryDialog() method should be replaced with getEditQueryWizard() -- WebTaskNewPage and WebTaskWizard should be made generic and reused by Jira plugin -- review WebTask, WebQuery, WebQueryHit and WebTaskExternalizer to see what can be generalized -- review WebQueryEditWizard, WebQueryWizard, WebQueryWizardPage, WebTaskNewPage, WebTaskWizard to see what can be generalized
(In reply to comment #22) > -- AbstractRepositoryConnector.openEditQueryDialog() method should be replaced > with getEditQueryWizard() I have opened bug 149921 to discuss changes to the query wizard api.
Mik, I guess we can close this now.
Closing as fixed, since there is no work left on this.
