Community
Participate
Working Groups
No search in report, I tried a big one, and it was very problematic to find something in it.
Yes, we need some sort of "find" dialog or feature for it. Current work-around is to use the Browser, but we're trying to minimize the need for that.
Unfortunately I don't think this will make it into 1.0.
Based on our discussion, the best place to add this for now would be to a new row on the editor's header. The row would only appear if the user hits Ctrl+F or a Find action on the form's toolbar, which would be a push button. Another option is to add the Find box to the toolbar instead of adding a whole additional row.
Created attachment 108278 [details] UI of "find" contributed a toggle action on the editor's header. When the action is toggled, including using key Ctrl+F, a textbox appears (it disapears when not toggled). I think its better than building a row which will take some real estates on the header. When programmers modified the text in the textbox, a find action will be activated, searching current Task Editor. I suggest also add a line to the left of the header to indicate the job is being done. The idea is pretty much like the "Find" box in the Task List View. Next step: build "find" under the hood.
Created attachment 108279 [details] mylyn/context/zip
Created attachment 108280 [details] screenshot after toggling the find button
Wow, that's cool. Can you move the patch to the sandbox for now?
Created attachment 108285 [details] moved the ui to sandbox now only the find button is availalbe in Bugzilla. And the patch also includes activating the editor context in TaskEditor, which is part of the functions (not being applied to the head yet) in bug 203670.
Created attachment 108286 [details] mylyn/context/zip
Created attachment 108287 [details] an update patch - ignore patch titled "move the ui to sandbox"
Owen, instead of implementing your own Find handler try reusing the global find action. You probably have to add this to TaskEditorActionContributor.registerGlobalHandlers() and delegate to the active task editor page for executing the find action.
Created attachment 108815 [details] the find patch Everything is working great except that the speed is a bit slow if too many comments. The main reason is that TaskEditorCommentPart.expendComment(..) will dispose the TextViewer instance when corresponding ExpandableComposite is close. I have to toggle all ExpandableComposite open before doing any search.
Created attachment 108816 [details] mylyn/context/zip
Created attachment 108817 [details] screenshot of typing "find" in bug 134165 * highlights all TextViewer, including those are editable, e.g. the new comment section * global Ctrl+F is available, even in context menu and Editor->find/replace * hides comments/sections that do not match any of the keyword
Created attachment 108980 [details] an updated patch using AdaptiveRefreshPolicy to improve user exp when keep typing the find string, the page will not refresh until finish the whole word. I like AdaptiveRefreshPolicy.textChanged(..) which schedules the job considering the text length
Created attachment 108981 [details] mylyn/context/zip
also we might need a line at the top of the page indicating the find job is in action
Owen, that's great stuff! I really like how the find integrates with the existing user interface. I committed the first patch a few days ago. Please update CVS and recut your follow-up patch against head. I have created bug 242969 for improving the performance when searching.
I checked out the screenshot. Very nice!
Created attachment 109112 [details] recut the patch against head request in comment 18
Thanks. I'll hold off merging this patch. I would like to figure out if we can make this fast enough (bug 242969) to avoid a delay when the user starts typing. For now I have changed the listener to only trigger the search when the enter key is pressed.
(In reply to comment #21) > Thanks. I'll hold off merging this patch. I would like to figure out if we can > make this fast enough (bug 242969) to avoid a delay when the user starts typing. > For now I have changed the listener to only trigger the search when the enter > key is pressed. oh btw, right now it won't delay since I added AdaptiveRefreshPolicy and the page will only refresh after the user has settle down the typing. But I agree we should start thinking about bug 242969.
Created attachment 109705 [details] the latest file of ExtensibleBugzillaTaskEditorPage.java I noticed my last patch is confilcting with the head, and the confilct still exists even if I recut it. So I send the file for review. Steffen, please take a look. I used multithread to delay the page refreshing, as commented at comment#15
Thanks Owen. I'll take a look at the patches. Make sure your workspace is sychronized with the latest from CVS. You can use the Synchronize view to resolve conflicts when updating from CVS.
(In reply to comment #24) > Thanks Owen. I'll take a look at the patches. Make sure your workspace is > sychronized with the latest from CVS. You can use the Synchronize view to > resolve conflicts when updating from CVS. Yes I did. But the wired thing is the conflict still exists. I think the problem is CVS is not atomic in synchronization.
(In reply to comment #25) > (In reply to comment #24) > > Thanks Owen. I'll take a look at the patches. Make sure your workspace is > > sychronized with the latest from CVS. You can use the Synchronize view to > > resolve conflicts when updating from CVS. > > Yes I did. But the wired thing is the conflict still exists. I think the problem > is CVS is not atomic in synchronization. I overrided the file and made changes on top of it, it just keeps complaining conflicts...
Steffen, this bug is ready for review. Although we need more time to fully finish bug 242969, my last patch fixed some performance issues, which can be used temporally.
The patch looks good. I would like to resolve bug 242969 before merging it (see comment 21) though. Since the current search implementation can block the UI for some time I think it's better to only trigger a search when enter is explicitly pressed.
Tentatively scheduling for 3.1 to include this in our planning, but we have to figure out whether a non-model based approach will scale.
Any update on this? Are you still waiting until bug 242969 is resolved? I seem to recall using this experimental "find" feature a few years ago without any performance issues. Is it still available from the sandbox?
Let's see if we can resurrect this.
After some experimentation, it looks like I am able to use the taskData to search through the comments (and later description) which cuts down on the performance time by 66%. The part that is expensive is when the comments are being opened, due to bug 70358 . Still need ideas on the best approach for searching through comments.
It actually isn't related to that bug. It turns out that almost half the time to expand comments is spent calling reflow(). Lily discovered that the find attempts to disable relfow by calling setReflow(false), but this has no effect since it gets set to true again when the comment viewers are created. If we use another mechanism to disable the reflow we could further improve the performance by a lot.
I have noticed a few issues with the old implementation, and changing some design methods fixed some problems and improved the performance greatly: 1) Search only through key parts: summary, private notes, description and comment parts instead of the entire task editor. 2) Expanding every comment is slow (due to the reflow() call), so instead of expanding all at once I first search through the task attributes to see if there is any match. If there isn't then don't expand the comment. 3) When there are groups in comments (Older, Recent) then expand the latest comments first if there are results (they are the ones not in a group), and if not then expand "Recent" group and so on, so at least some result is presented to the user upon search. If there aren't any results in a group then don't expand that group. 4) If both the latest comments and groups contained results, then the group will not be expanded but a link will appear beside the group header that indicates that there are results in the group. Clicking on that link will expand the group and will highlight the results. (Link will disappear after clicking on it) 5) If a comment is expanded already by the user then it will not collapse the comment if there isn't a result. (same with other parts) 6) After every search, there was a call to viewer.refresh() which causes a slight formatting change in the comments. If comments contain quotation, then the first line of the quotation (starts with "(In reply to..)") will become unquoted. By not calling that method and instead setting a blank styleRange to StyledText fixes that issue. 7) Instead of using FindReplaceDocumentAdapter to find the region to highlight, I use string indexOf on StyledText.getText() which is a lot faster. 8) Mouse hover over a link within a comment will wipe out the highlights in that comment. I have yet to find a solution for this.
I have submitted a patch: https://git.eclipse.org/r/#/c/15793/ which depends on some changes in the mylyn tasks, so I pushed a patch for that as well: https://git.eclipse.org/r/#/c/15791/
We will need to file a CQ to move this code to the tasks repository, but I have pushed the following additional reviews, building on Lily's. Once we have approval to move the code, it will be trivial to enable this functionality for all connectors. 17138: 134165: improve readability of task editor find implementation [Ic3c9b515] https://git.eclipse.org/r/#/c/17138/ 17188: 134165: move task editor find implementation to separate class [I348a4b18] https://git.eclipse.org/r/#/c/17188/
*** Bug 242969 has been marked as a duplicate of this bug. ***
Moving to 3.11. We made great progress on this in this release cycle. I think we should be able to merge it early in the 3.11 cycle.
It sounds like we don't need to do a move review (a type of restructuring review) to move this amount of code (approximately 300 lines). It says at http://www.eclipse.org/projects/dev_process/development_process.php#6_3_8_Restructuring_Review that: bq. A Restructuring Review may include the movement of significant chunks of code. A move is considered significant if it has an impact on the community (i.e. if segments of the community will notice that the code has moved). This may include entire projects, bundles, and features, but likely excludes small fragments, code snippets and individual files Steffen, do you think we can just move the code without going through a move review?
(In reply to comment #39) > It sounds like we don't need to do a move review (a type of restructuring > review) to move this amount of code (approximately 300 lines). > > Steffen, do you think we can just move the code without going through a move > review? Sounds fine if it's a small amount of code like that and we'll note it in the N&N anyways to make the community aware.
I've moved the code to o.e.m.tasks. Marking resolved!
