Bug 173793 - Consider adding a Preview page to the Organize Manifest tool
Summary: Consider adding a Preview page to the Organize Manifest tool
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: 3.4 M1   Edit
Assignee: Adam Archer CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed, noteworthy
Depends on: 198318
Blocks:
  Show dependency tree
 
Reported: 2007-02-11 13:45 EST by Simon Archer CLA
Modified: 2007-08-13 10:56 EDT (History)
3 users (show)

See Also:


Attachments
lightly tested prototype (49.31 KB, patch)
2007-07-23 20:35 EDT, Adam Archer CLA
no flags Details | Diff
improved patch (55.13 KB, patch)
2007-07-24 16:34 EDT, Adam Archer CLA
no flags Details | Diff
further improved patch (55.01 KB, patch)
2007-07-25 11:28 EDT, Adam Archer CLA
no flags Details | Diff
addresses issues in comment 6 and adds functionality in comment 7 (56.72 KB, patch)
2007-07-26 14:56 EDT, Adam Archer CLA
no flags Details | Diff
more improvements (59.51 KB, patch)
2007-07-26 17:06 EDT, Adam Archer CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Archer CLA 2007-02-11 13:45:49 EST
While the Organize Manifest tool is very helpful, it would be more helpful if the user could review the changes that will be made before committing themselves.  Today the only way to achieve this is to make sure that the project is commited to the repository before running the tool, and then comparing the changes with the stream.

Please consider adding a preview page, which might be rather complicated since there can be multiple files updated and of course you can select multiple projects.  Tricky, perhaps.
Comment 1 Chris Aniszczyk CLA 2007-02-11 14:19:19 EST
cool, may look at it once PDE survives M5
Comment 2 Wassim Melhem CLA 2007-07-22 18:33:13 EDT
Assigning to Adam.   Brian to advise/review.
Comment 3 Adam Archer CLA 2007-07-23 20:35:04 EDT
Created attachment 74406 [details]
lightly tested prototype
Comment 4 Adam Archer CLA 2007-07-24 16:34:22 EDT
Created attachment 74506 [details]
improved patch

This patch:

- Cleans up some inconsistencies with the wizard settings
- Removes some unnecessary references and applies some best practices
- Adds context highlighting for changes to manifest files and properties files (still not available for plugin.xml files due to bug 192863)
- Removes the nondescript top level change item in the preview viewer (by marking the top level CompositeChange as synthetic)

This patch still needs heavy testing to ensure no functionality has been broken and that the previewing works for all edge cases.
Comment 5 Adam Archer CLA 2007-07-25 11:28:18 EDT
Created attachment 74578 [details]
further improved patch

Removed a couple unnecessary instance variables and corrected one unexternalized String.
Comment 6 Brian Bauman CLA 2007-07-25 17:18:03 EDT
It is my fault this bug has not been fixed yet.  I have not been able to keep up with Adam :)

When we started on the preview page, I told Adam to start easy and work up from there.  During some final testing I seem to have come across a slight quirk in an advance case.

To reproduce:
1. Open a Manifest.MF file which has been organize in the Manifest Editor.
2. From the Dependencies Tab, add a dependency to a new unused bundle. Do NOT save the file.
3. Run the Organize Manifest action, select preview.  When prompted to save, select No.

Notice how there won't be any changes.  Do the same steps again, except in #2 add the dependency from the Manifest.MF source page (again, don't save and click No at the prompt).  This time you will see the plug-in added in step #2 shows up in the preview.

This happens because the modification to the form editor creates a TextChange.  The TextChange is not applied until the user switches to the source page.  So when we commit the buffers in the PDEModelUtility, the buffer is not update with the TextChange.

There is another small detail.  When we modify the Manifest.MF source page and run the OrganizeManifestAction without saving, the editor is saved upon the user selecting preview.  If we can, we need to try to not save the editor until the user clicks OK.  
Comment 7 Wassim Melhem CLA 2007-07-26 00:07:06 EDT
Adam, I released the plugin.xml compare so you could go ahead with previewing plugin.xml changes.

Unfortunately, the PDEUIMessages changes in your patch are now out of sync.
Comment 8 Adam Archer CLA 2007-07-26 14:56:42 EDT
Created attachment 74719 [details]
addresses issues in comment 6 and adds functionality in comment 7
Comment 9 Adam Archer CLA 2007-07-26 17:06:10 EDT
Created attachment 74731 [details]
more improvements

Added a check for fragment.xml as well as plugin.xml to set the text type to PLUGIN2. This way fragments are also handled by the PluginContentMergeViewer.

Also refactored both RenamePluginRefactor and OrganizeManifestRefactor into PDERefactor since they were identical.
Comment 10 Brian Bauman CLA 2007-07-27 15:26:37 EDT
Oh so pretty!

I really liked the way you went back and refactored RenamePluginRefactor to PDERefactor so it can be reused, very thorough.  And the use case with a dirty editor added only a slight bit of code, very elegant.

The only modification I made was in PDEModelUtility.  When creating the TextFileChange, I called TextFileChange.setSaveMode(TextFileChange.FORCE_SAVE).  This caused dirty editors to be saved after the change was applied.  This was just for consistency with the old way it worked.

Adam, I am impressed how fast you were able to do this bug with very little help or guidance, good job!
Comment 11 Wassim Melhem CLA 2007-07-27 21:42:21 EDT
It certainly is pretty.  However, it is not quite right.

The checkbox tree acts funny.

When a node has two children, and one of them gets deselected, the parent node currently gets deselected.  This is incorrect.

If a parent node has n children, and m nodes are selected where 1 <= m < n, the parent node should have a green square, not completely lose its checked status.  See the build page of the product editor and the Plug-ins tab of the Eclipse app launcher for examples.
Comment 12 Adam Archer CLA 2007-07-30 15:34:24 EDT
Reopening to address the issue in comment 11.
Comment 13 Wassim Melhem CLA 2007-08-07 01:40:10 EDT
closing since the tree is fine now.
Comment 14 Chris Aniszczyk CLA 2007-08-13 10:56:27 EDT
adding noteworthy keyword