Bugzilla – Full Text Bug Listing
| Summary: | Refactor searching and displaying of resources | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Andrew Eisenberg <andrew.eisenberg> |
| Component: | Client | Assignee: | John Arthorne <john.arthorne> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | john.arthorne, kdevolder, libingw, simon_kaegi |
| Version: | 0.4 | ||
| Target Milestone: | 0.4 RC1 | ||
| Hardware: | PC | ||
| OS: | Mac OS X - Carbon (unsup.) | ||
| Whiteboard: | |||
|
Description
Andrew Eisenberg
Andrew, you can refer to searchExplorer.js. I know it is not perfect but if you have further questions please just let me know. There you will see the explorer-model-renderer pattern. The model we are using is tree structure, which is 2-level here. In your case if you just want a flat structure you can refer to SearchReportRenderer in this file. The report renderer can be triggered by search->replace->report workflow. Basically the idea of the global search result page is to wrap up the result json and use the explorer to render an html table into a given DIV. In your case you may want to do the similar thing. As a start point you can refer to "explorer.startUp();" in searchResults.js and dig into the explorer. I have Bug 369738 planned for 0.4. I was thinking it is generic to any tree model iteration, where it is currently needed and will be consumed in search result and preview. Andrew, if you think you will need more complicated iterating logic other than just walking through a flat list, you are welcomed to put your input there. Thanks for the pointer. Kris De Volder and I have been discussing some potential solutions. One thing that we are struggling with is how generic we should make our solution. The general solution is to provide something like org.eclipse.jface.viewers.ILabelProvider for Orion to render individual entries (search result, favorite, git log entry, etc). And then there must be something else, like a viewer that can put these things into a table or a list. But I am reluctant to produce something overly general at this early stage with my small understanding of Orion. It looks like explorer.js is an attempt to be a sort of viewer for a bunch of items. I'll have a deeper look at it. I have worked on this with Kris De Volder on branch Bug369892 at his github clone of the client. We did our best to squash the commits into something sensible, but we also wanted to keep author tags where it made sense. Kris's original commit to initially separate the rendering from the searchClient. This created the searchRenderer.js file. All of this code was written by Kris. https://github.com/kdvolder/orion.client/commit/2804e6fd2ba6c2a7564c0e65394fee06718c4517 My commit to make the favorites use searchRenderer.js inside of the open resources dialog. All this code was written by me. https://github.com/kdvolder/orion.client/commit/79654c6c6feaa9f56e2307d19a1d50f7ee1f9e7c Kris's cleanup commit to make a few extra cleanups. All of this code was written by Kris. https://github.com/kdvolder/orion.client/commit/7963d7cc9771adcbd8e7abd3aab62a29bc9d0cfb These commits should be applied in the order above, and they have been rebased off of master as of a few hours ago. A few comments about what we have done: 1. extracted the pieces of searchClient that deal with rendering HTML into searchRenderer.js 2. We removed lots of code that is unused, eg- highlighting 3. We removed filtering out by excludeFile for two reasons: a) it didn't really fit with the separation of search logic from UI logic that we were trying to achieve, and b) neither of us thought that this functionality was useful. The excludeFile field is only used when doing CTRL-H. It excludes the current file from the search, but both of us preferred seeing the current file in that list, so we did not try to re-implement that feature. 4. Implemented some new tests. I ran them on google chrome and firefox on MacOS, but nowhere else. 5. I understand that it is better to give one nice commit than 3 messy ones that add up to a nice commit, but our understanding is that we need to keep the author tags correct. If you'd prefer one big commit, we can certainly squash and do something cleaner. Multiple commits is fine. I'll take a look at this later today. I haven't looked at how Susan implemented the fix for Bug 370050, but there may be some conflicts since it likely touches some of the same code that has been refactored by the patches in this bug. I can rebase the patch on top of Bug 370050 if there are any problems. I have released these three commits to master: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=71fb205e87e3b18d3d3769036b95e4e03743aad6 http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=07335a519d7aa649fed734d62455083df6cf493d http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=0641e29dc82de224aa15e20dcaa5cc3d96549021 There were conflicts with both Susan's fix and some fixes from Mark. However I think I resolved all the conflicts and my testing showed things are working. This did whack Susan's fix for bug 370050. Since the rendering code is now generic it's not so easy to add the icon for favorites. However rather than deal with another painful merge I am overwriting Susan's change and I will reopen that bug. Please quickly verify my merges and mark this fixed if you are satisfied with it. Thanks for having a look at this. I peeked at the commits and they look fine. I think I saw you add a new parameter to mSearchRenderer.makeRenderFunction for hideSummaries (or is this something that Kris did?). Did you make any other significant changes? A way to put Susan's fix back in would be to include some kind of callback that knows how to include images while rendering. I can have a look at that. ps- I see there is a review flag with my name on it. Should I be changing it to a '+' when I am happy with this? (In reply to comment #8) > Thanks for having a look at this. I peeked at the commits and they look fine. > I think I saw you add a new parameter to mSearchRenderer.makeRenderFunction for > hideSummaries (or is this something that Kris did?). Did you make any other > significant changes? I didn't add that argument or make any changes. I just merged some conflicting changes and verified that everything worked. > A way to put Susan's fix back in would be to include some kind of callback that > knows how to include images while rendering. I can have a look at that. I reopened bug 370050 for that. This has been deployed to orion.eclipse.org, and now Open Resource is completely broken. This was working fine for me in my deployed self-hosting site on orion.eclipse.org so I am investigating what happened here. I see the same problem. It's odd, but when I go into the Chrome dev tools, I don't see that OpenResourcesDialog.js has been loaded at all. It doesn't show up in the network, resources, or scripts tab. I have fixed this: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=ad16689cb48a68c75eeb2117cdf0ae72ede91a49 However I don't understand why. Module imports don't seem to be working at build-time for OpenResourceDialog.js. I have refactored to pass the module as a constructor argument instead, and this is working for me. I will build and deploy to verify. I have opened bug 370600 to get to the bottom of the module dependency problem. Grr.. still broken on orion.eclipse.org. I'll need to debug our build/minification code on Monday with Simon. Assigning to me for now. I have been debugging the built code, and I'm starting to think this is a require.js bug. 1) I put a breakpoint inside the module definition for "orion/globalCommands", which is referencing "orion/searchRenderer". 2) I put a breakpoint inside the definition of the "orion/searchRenderer" 1) is getting hit before 2). So, the code that requires the renderer gets executed before the search renderer module even gets defined. Of course the required module is passed in as null and the execution fails. We have fairly deeply nested modules here and I have a suspicion we are hitting: https://github.com/jrburke/requirejs/issues/173 This is supposedly fixed in Require.js version 1.0.5. I think to get us going again I'll have to inline the rendering code again, and revisit after we moved up to require.js 1.0.5. I have merged the modules, but kept the separation of concerns between rendering and searching code: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=f53027474faf9a1b7c14d9fdb8972a111d66c1b1 Note bug 370600 is open to figure out the module loading problem. I added my comment #15 words there and I'll follow up there. Verified in I20120206-1525. |