Bugzilla – Full Text Bug Listing
| Summary: | consider removing Current subsection | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Steffen Pingel <steffen.pingel> |
| Component: | Mylyn | Assignee: | Steffen Pingel <steffen.pingel> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | david.shepherd, jingweno, mik.kersten |
| Version: | unspecified | ||
| Target Milestone: | 3.1 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | |||
| Bug Blocks: | 238038 | ||
| Attachments: | |||
|
Description
Steffen Pingel
I have no idea until I get a chance to use it, so will wait until it's in HEAD before commenting. Related to this bug: I just pressed the + button on all comments when the comments were showing recent and current. This behaved as expected, expanding all comments. However, pressing - to fold all comments still leaves each section (recent and current) expanded, which is not what I expected. I would prefer if the global - folded everything, not just the comments. Is this the intended behaviour? Just wanted to give some feedback... looks cool otherwise! Created attachment 115766 [details] a patch that fixed the proposition comment #2 Great point David! This is something we didn't cover before. The initial intention was to keep the existing way: the "old" collapse all only close the comments without closing the section. So now we only close all the comments without closing the subsections. Steffen, in this patch, I implemented to close all the subsections when pressing the collapse all button. Another proposition would be to only close the "older" & the "recent" and leave "current" inacative, since our goal is to always keep "current" open? Sorry Owen, I missed that change. I have just committed a change that removes the current section. We'll need to figure out how to adjust the behavior of the expand buttons based on these changes. (In reply to comment #4) > Sorry Owen, I missed that change. I have just committed a change that removes > the current section. We'll need to figure out how to adjust the behavior of the > expand buttons based on these changes. n/m. I think it makes sense to collapse all subsections when clicking the "collapse-all" button though. removing the current section will definitely make the design simpler. But programmers couldn't expend-all current comments at once (maybe add another button on the main section toolbar). Besides, when all subsections + comments are expended, there are no boundary between the "recent" comments and the "current" comments. Thats a bit confusing. I think that the expand all button should first expand all current comments on the first click and then on a second click expand all comments. Otherwise users can't expand all current comments, and expanding all comments is too expensive in terms of screen size. Created attachment 117227 [details]
Patch to progressively expand the comments when the expand all button is pressed.
First click expands current, second click expands recent, third expands older. Expansion is sensitive to the current expansion state of the comments. Unfortunately, there is no clean way to access the Recent and Older sections and so the code to get these sections is not as clean as it should be.
Created attachment 117228 [details]
mylyn/context/zip
I had to make a few changes to the current implementation to improve performance. Generally the button should work based on the state of the UI and not rely on particular sections to be present. Have you tried iterating over the widgets to check if all comments in the current section are expanded to determine the behavior of the button? I would prefer that over keeping separate state in an integer which is tricky to synchronize when the user collapses or expands comment/sections manually. (In reply to comment #9) > I had to make a few changes to the current implementation to improve > performance. The pre-patch implementation? or was the patch causing performance problems? > Generally the button should work based on the state of the UI and > not rely on particular sections to be present. The button does work based on the state of the UI, I can work on making it not rely on particular sections. > Have you tried iterating over the > widgets to check if all comments in the current section are expanded to > determine the behavior of the button? This is very similar to what the code does. The code checks the state of the UI when the button is pushed by checking the expansion state of the "older" and "recent" sections. It does not check individual comments, however, only the section. > I would prefer that over keeping separate > state in an integer which is tricky to synchronize when the user collapses or > expands comment/sections manually. The state is not "kept" in an integer, it is recalculated each time the button is pushed by examining the expansion state of the UI. From you comments, it seems the main complaint is having the code rely on the presence of certain sections. If I make the code more generic by removing references to these sections will you have any other issues with the patch? If so, what (so I can address them at the same time). Created attachment 117345 [details]
Patch to change the behaviour of +
Try #2, should be more generic.
Created attachment 117346 [details]
mylyn/context/zip
On a related note, we should also change the behaviour of -, but waiting until I get this patch through before working on that one.. The performance changes were unrelated to the patch. I just noticed slowness when I tested expanding comments. Patch looks good, I misread the first patch you submitted not realizing the the integer state is recalculated each time the button is pressed. One thing I noticed with the latest patch is that it expands all comments if I click the button when the Comments section is collapsed. Created attachment 117359 [details]
Patch to change the + behaviour
Created attachment 117360 [details]
mylyn/context/zip
I think you should only count a section as expanded if all comments in that section are expanded. The counting should also be based on the same assumption that is made in expandAllComments(), i.e. sections are counted bottom up. Consider this scenario for instance: Collapse all and expand the Older section, then press the button. As a result of pressing the expand button Older will be collapsed and Recent and all current comments will be expanded. Created attachment 117503 [details]
Patch v3 to change + behavior
Should fix some of the issues we discussed. However, this code is starting to grow more complex as there are several special cases and it is not easy to access the UI widgets directly. As Steffen suggested, this class should be put on the list of classes to refactor in the near future.
Created attachment 117504 [details]
mylyn/context/zip
The patch looks good. I am hesitant to apply it though as it tracks comment groupings by number ranges. After looking at the class I now believe that it would be better to refactor the data structures first which currently don't capture the mapping between the data model (attributes) and UI representation (subsections, comment editors etc.). This would also help with other e.g. model-based search, bug 242969). I agree that tracking the comment groupings by number ranges is not desirable. What would it take to refactor the data structures, in terms of time and effort? Steffen? I think this will be straight forward. I have a drawing on my white board :). Let's talk about it today. I have refactored the class and implemented incremental collapse / expand of comments. Please open a new bug if further enhancements are needed. Great! I just tested it out and it seems to perform a lot faster as well, did you do any profiling? For a bug with 90 comments it used to take ~10 seconds to expand and now it takes 0.5 seconds. Nice! I also noticed that the "-" button is the inverse of the "+" button now, good. (In reply to comment #24) > I have refactored the class and implemented incremental collapse / expand of > comments. Please open a new bug if further enhancements are needed. > Cool stuff guys! Nice hack in TaskEditorCommentPart.expandAllComments()! Just curious: is the user story changed? now the "current" group takes the last 12 comments instead of taking the comments since current user's last comment... The current strategy will still only show the comments since the user last commented or the last 12 (whichever number is lower). Created attachment 117956 [details] screenshot of the new comment group strategy (bug 238038) But it seems "showing the last 12 comments" overshadow "showing the comments since the user last commented" (always showing the last 12 comments)? In the screenshot, should it only display comment #70 and #71 instead of the last 12 comments (comments since the user last commented has a higher priority)? Good point. That looks like a bug. Time to write test cases :). (In reply to comment #29) > Good point. That looks like a bug. Time to write test cases :). see bug 244371 Created attachment 118208 [details]
shared file
A shared file created from: new.png in folder 248057
|