Community
Participate
Working Groups
The image browser view from bug 386197 is broken for source Workspace. WorkspaceRepository#populateCache(IProgressMonitor) loops endlessly if there's a project without a manifest, and I also think it doesn't work if there's more than one bundle in the workspace. Moreover, the hand-made resource traversal is a no-go. IResourceProxyVisitor is the only acceptable way to visit the whole workspace.
Created attachment 227225 [details] patch fixing the infinite scanning og workspace projects You are right, the traversal of the workspace did not work. The patch will fix this. While I do see your point to use IResourceProxyVisitor to scan the workspace I do not know how to apply this pattern here fore the whole workspace. We do not scan the whole workspace at a certain time. We just scan projects until we gathered enough icons to display for the user. The user can continue scanning on demand. With an IResourceProxyVisitor we would have to cache already scanned projects to avoid re-scanning them. What we could do is use an IResourceProxyVisitor for scanning individual projects. What do you think?
Created attachment 227240 [details] Patch implementing visitor pattern for projects Added a patch to apply the visitor pattern when scanning single projects. This one includes the original patch
(In reply to comment #2) Thanks for the patch. One IResourceProxyVisitor per project is also fine. But you don't check monitor.isCanceled() any more. The IResourceProxyVisitor is very quick, so unless the visit(..) method is expensive, we don't need to check for each file. I suggest you: (1) get rid of the toLowerCase() call, which allocates a new String for each resource in the workspace. If AbstractRepository#isImageName(..) uses String#regionMatches(true, ..), then the comparison comes at almost no cost. (2) add a monitor.isCanceled() check after the isImageName check succeeded. If the monitor is canceled, throw an OperationCanceledException.
Created attachment 227250 [details] Patch implementing comment 3 implemented as requested. I had to introduce a "canceled" flag for the monitor as I guess there is no other way to stop the visitor.
(In reply to comment #4) > I had to introduce a "canceled" flag for the monitor as I guess there is no > other way to stop the visitor. I suggested to throw an OperationCanceledException, and I still think that's the right way to stop the visitor. By the way: Your field access to fIsCanceled is comparable to just checking monitor.isCanceled() every time (which is also just a method invocation + a field access). In searchJarFile(..), there's another unnecessary call to toLowerCase().
Created attachment 227291 [details] Patch implementing comment 5 > I suggested to throw an OperationCanceledException Did miss that in your previous comment. Now implemented as requested. > In searchJarFile(..), there's another unnecessary call to toLowerCase(). Was already replaced in the previous patch. I did remove a toLowerCase() in SaveToWorkbench.getImageType() and I switched the default type from NONE to PNG. Otherwise not entering a file extension would result in no image being saved (without any user feedback). Still the simple save dialog does not make me happy as the user does not know that entering different file extensions result in different encodings of the saved file.
After applying the patch and displaying images from the workspace, clicking on them does not find the correct path. Path ends up blank which makes the reference string incorrect as well.
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=cbe106d804fe3cd3e6d7b1da9e7921db33f22a89 Applied the patch with copyright updates and a fix to use the file path not the project path.
