Bugzilla – Full Text Bug Listing
| Summary: | Improve query tooltip layout | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Eugene Kuleshov <ekuleshov> | ||||||||||||||||||||||||||||||||
| Component: | Mylyn | Assignee: | Eugene Kuleshov <ekuleshov> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||||||||||||
| Priority: | P4 | CC: | robert.elves, steffen.pingel | ||||||||||||||||||||||||||||||||
| Version: | unspecified | ||||||||||||||||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Eugene Kuleshov
Please also add number of open issues, so it won't be necessary to substract 4 digit numbers to figure out how many open issues appear on query: Open [x] / Completed [x] / Total [x] Created attachment 71690 [details]
refactor TaskListToolTipHandler
The patch cleans up the tooltip code. I tried to keep the tooltip similar to how it was but it is now easily changeable so we can discuss and play with layout refactorings.
Mik, please review and merge the patch. *** Bug 193029 has been marked as a duplicate of this bug. *** Created attachment 71692 [details]
mylar/context/zip
Created attachment 71763 [details]
updated patch
Update fixes a NullPointerException and displays the start date for ScheduledTaskContainers.
Created attachment 71764 [details]
mylar/context/zip
Patch applied, very nice Steffen. I put the repository URL and details on the same line because the extra line was not using horizontal space well enough in common cases. I also added the key after the kind. Please review changes. Additional improvements can be added to the UI nits. Created attachment 71772 [details]
mylyn/context/zip
The layout of the query tooltip is now missing a new line and inconsistent with the task tooltip (it does not use brackets for the repository). Is that a typo or a new term: "Synchronizatied"? I'll provide a fix later. Created attachment 71782 [details]
fixes newline issue and tweaks ui
Guys, can we please have first line of the tooltip for query hits to match the text label shown in the task list? So, it would be easy to pick it up and it would behave consistent to other tooltips in the IDE and would allow to deal with bug about tooltip positioning related to the text label instead of mouse pointer (can't find bug number after migration of my task list). Also, I see that task tooltip has info about number of completed tasks, but that stuff should be only shown for query and category nodes. (In reply to comment #12) > Guys, can we please have first line of the tooltip for query hits to match the > text label shown in the task list? I am fine with that as long as the full url shows up in the tooltip. > So, it would be easy to pick it up and it > would behave consistent to other tooltips in the IDE and would allow to deal > with bug about tooltip positioning related to the text label instead of mouse > pointer (can't find bug number after migration of my task list). bug 189313 > Also, I see that task tooltip has info about number of completed tasks, but that > stuff should be only shown for query and category nodes. It's only shown for tasks that have subtasks which makes sense to me. (In reply to comment #13) > I am fine with that as long as the full url shows up in the tooltip. Out of curiosity, why do you need url for task in the tooltip? It can't be copied or anything... I have multiple repositories on the same host and the tooltip is the only way to distinguish them without labeling the repositories. Need to fix: The task key displayed for local tasks is "null". (In reply to comment #13) > (In reply to comment #12) > > Guys, can we please have first line of the tooltip for query hits to match the > > text label shown in the task list? > > I am fine with that as long as the full url shows up in the tooltip. +1 this is important for consistency reasons. +1 for having the full URL for queries and the label (or full URL if none) for tasks. Then between the label and the tooltip all the relevant information is accessible without opening the Task Repositories view. Steffen: I'll wait for a patch. Created attachment 71811 [details] check for null task key (In reply to comment #17) > (In reply to comment #13) > > (In reply to comment #12) > > > Guys, can we please have first line of the tooltip for query hits to match > the > > > text label shown in the task list? > > > > I am fine with that as long as the full url shows up in the tooltip. > > +1 this is important for consistency reasons. That would mean we need to display the full url in the task list as well. > Steffen: I'll wait for a patch. I have attached what I have now but the first line does not match the label for queries and for scheduled task containers. BTW, I am not sure if checking the task key for null is a good thing. I guess there could be many places in the UI where that needs to be done for local tasks. Maybe it would be better to return a meaningful value or "" in LocalTask. (In reply to comment #17) > +1 for having the full URL for queries and the label (or full URL if none) > for tasks. Then between the label and the tooltip all the relevant > information is accessible without opening the Task Repositories view. Eh? Are you sure it is a good idea to show something like this in the tooltip? https://bugs.eclipse.org/bugs/buglist.cgi?short_desc_type=allwordssubstr&short_desc=&product=Mylyn&component=Bugzilla&component=Core&component=Doc&component=Java&component=Jira&component=Monitor&component=Tasks&component=Trac&component=UI&component=Web&component=XML&component=XPlanner&long_desc_type=allwordssubstr&long_desc=&order=Importance (In reply to comment #18) > > +1 this is important for consistency reasons. > That would mean we need to display the full url in the task list as well. I thought that one is for label consistency? BTW, there was another enhancement request, which I also can't find now. It was about showing number of open issues for query/category, so user won't have to subtract number of completed tasks from the totals... (In reply to comment #19) > Eh? Are you sure it is a good idea to show something like this in the tooltip? > > https://bugs.eclipse.org/bugs/buglist.cgi?short_desc_type=allwordssubstr&short_desc=&product=Mylyn&component=Bugzilla&component=Core&component=Doc&component=Java&component=Jira&component=Monitor&component=Tasks&component=Trac&component=UI&component=Web&component=XML&component=XPlanner&long_desc_type=allwordssubstr&long_desc=&order=Importance Sorry, I was talking about the repository URL, not the query URL. > (In reply to comment #18) > > > +1 this is important for consistency reasons. > > That would mean we need to display the full url in the task list as well. > > I thought that one is for label consistency? I am completely confused now. Please provide a patch that layouts the tooltip according to your needs. (In reply to comment #20) > BTW, there was another enhancement request, which I also can't find now. It was > about showing number of open issues for query/category, so user won't have to > subtract number of completed tasks from the totals... Yes, include a suggestion how to do that in your patch. Mik, could you apply the last patch I posted so we can iterate further on layout changes? Getting a conflict on that patch. Please re-spin. Rob: I'm stuck in externalizer land, please apply. Created attachment 71971 [details]
updated patch
Patch applied. The tooltip now contains all information I am interested in. Eugene, please post a patch if you need changes to the current layout. (In reply to comment #26) > The tooltip now contains all information I am interested in. Good for you. (In reply to comment #27) > Good for you. ? (In reply to comment #26) > The tooltip now contains all information I am interested in. Eugene, please post > a patch if you need changes to the current layout. Looking great. Some nits: * On a category the "Completed" starts at the very left of the tooltip and looks weird. Maybe that label should be centered sinece the progress composite spans the whole tooltip? * I don't think we need "Comment by foo@bar:", "foo@bar:" should be sufficient and if not we can change the icon to be the little person. * Tasks with subtasks will show a progress bar, they should not. (In reply to comment #28) > > Good for you. > ? As I understand Steffen crafted tooltip for his needs. So, I just want to make sure that issues pointed out by reporter are addressed. Created attachment 72100 [details] more tooltip tweaks > As I understand Steffen crafted tooltip for his needs. So, I just want to make > sure that issues pointed out by reporter are addressed. Sorry, shouldn't have closed the issue. I originally opened bug 193029 for the changes I needed but it turned out that the TaskListToolTipHandler needed refactoring which seemed closer related to this report. > * On a category the "Completed" starts at the very left of the tooltip and looks > weird. Maybe that label should be centered sinece the progress composite spans > the whole tooltip? I centered it but that looks a bit weird as well. > * I don't think we need "Comment by foo@bar:", "foo@bar:" should be sufficient > and if not we can change the icon to be the little person. That text is provided by the tasks framework and the same as on the incoming notificiaton. This would require additional hacking if it needed to be changed. > * Tasks with subtasks will show a progress bar, they should not. Removed it although I kind of liked it for JIRA where sub tasks can be actual sub tasks (and not dependent tasks). Other changes in the patch: * Moved url into title for queries * Added summary to title for ScheduledTaskContainers * Added icon for ScheduledTaskContainers The tooltip title should now be consistent with the text, except: - it displays the full url instead of the short url for queries - it displays the start date for ScheduledTaskContainers I am done here. Created attachment 72101 [details]
mylyn/context/zip
Patch applied. Regarding the subtasks thing, I see your point now so consider putting it back. (In reply to comment #32) > Patch applied. Regarding the subtasks thing, I see your point now so consider > putting it back. It only makes sense if the subtask have a true "contains" relationship to the parent task which is not generally the case. I am in favor of leaving it the way it is now. Great stuff Steffen. If there is more to be done let's open up new bug reports unless I missed something that's worth leaving this one open for. Either way the current status looks good for 2.0. Technically there should be a containment relationship which is why we would be able to show it that way in the Task List, but yup, it can be weaker and I agree that it's best left of for now. The centering of "completed" does look better to me. We can improve further on that down the road by visually separating the progress section with a faint horizontal line. Guys, it is good that you are happy with result of your work, but really, that wasn't what original report been requesting to fix: "In this layout error details are not visually separated and hard to read." It seems like error is still in the middle and issue outlined in comment #1 is also present. Are you saying that I should open new bug report for those? Also, maybe it is just me, but I think "completed" row look really odd when centered. It is even worse when tooltip is wider then task list (that happens when you have an error message up there). Generally I saw very few cases when centering widgets is working ok (especially for people who read from left to right). The current improvements is all that we can commit to for 2.0 and this bug is set for 2.0. Steffen did some very good work on this, in my opinion has made significant improvements on "In this layout error details are not visually separated and hard to read" and bug so I wanted that to be reflected in it being resolved. The stuff in comment#1 could make sense to do so please file another bug, but I'm not yet convinced it is worth it because it will make that text 2-3x longer. For other improvements you want to see please do file another bug. (In reply to comment #36) > Steffen did some very good work on this, in my opinion has made > significant improvements on "In this layout error details are not visually > separated and hard to read" Eh. I must be missed that part. In last dev build is appear in the middle of the tooltip and hence not really stand apart. Steffen: for containers I think that there is one little thing you could additionally do to improve visual separation, and that's to add the Synchronize icon to the "Synchronized:" row. Leading hanging icons help the eye distinguish the entry as a separate row. I agree with Eugene that I didn't address the issues requested by this report and posted patches to the wrong bug. I apologized for that in comment #30. I also agree that further improvements could be made to the layout and better visual separation would help but I can't spare the energy for lengthy discussions at this point. That's fine with me. This bug does capture some valuable points and tradeoffs. Still great that you got all these improvements in for 2.0 Steffen! Created attachment 72389 [details] added info about changed attributes I think issue from the task description is already resolved in bug 194970, but to continue tradition, here is another patch. It adds info about changed attributes, which is really handy and helps with reviewing incoming changes in Task List, eliminating need in opening task editor in many cases. Created attachment 72390 [details]
mylyn/context/zip
Patch applied and some clean-up done to avoid exposing exposing internals as much as possible since TasksUiPlugin is not the right place for this functionality and it will need to move post 2.0. (In reply to comment #43) > Patch applied and some clean-up done to avoid exposing exposing internals as > much as possible since TasksUiPlugin is not the right place for this > functionality and it will need to move post 2.0. Great! Thanks Mik. Initially I had all that info inside description, but figured that it would affect notifications (I don't use them), so I put it into a separate method, but probably should have added flag on incoming notification class (i.e. to show short or long info), though notification UI could just cut description to few lines... This patch has not been verified sufficiently and has at least the following problems which should not go out with tonight's RC4: * Modified dates are included in the diff. * Tooltip can grow very tall (e.g. formatting attachment), probably from blank newlines from things like attachments. Could be worse if a bunch of attributes change. (In reply to comment #45) > * Modified dates are included in the diff. Do you suggest to hardcode that attribute to be skipped? > * Tooltip can grow very tall (e.g. formatting attachment), > probably from blank newlines from things like attachments. What you mean here? > Could be worse if a bunch of attributes change. That is the point. You can see changed attributes. Though we could limit it to say, 5 attributes (10 lines). (In reply to comment #46) > Do you suggest to hardcode that attribute to be skipped? Yes, this attribute delta is meaningless and should be skipped. > What you mean here? When a context is attached the tooltip will have 2-4 blank lines. > That is the point. You can see changed attributes. Though we could limit it to > say, 5 attributes (10 lines). They need to have a max length and be elided (i.e. "...") after that. 4 is good number to limit to because it is the most a person can quickly glean and why we set the incoming notifications to a max of 4. Created attachment 72447 [details] few improvements (In reply to comment #47) > > Do you suggest to hardcode that attribute to be skipped? > Yes, this attribute delta is meaningless and should be skipped. Problem is that those attributes are not standard and isHidden() method is not reliable and currently twisted. It is used to hide attribute from common attrbutes section, i.e. when attribute need to be rendered manually or appear somewhere else (i.e. "summary"). Anyways, I've added some hack to hide bugzilla-specific stuff. > > What you mean here? > When a context is attached the tooltip will have 2-4 blank lines. That is an old behavor, but I cleaned it, by replacing all line breaks with spaces. May need to use something more sophisticated. > They need to have a max length and be elided (i.e. "...") after that. 4 is > good number to limit to because it is the most a person can quickly glean and > why we set the incoming notifications to a max of 4. Attributes didn't meant to appear on notification, and for tooltip more details is important. Anyways, if you insist, I've added restriction to show up to 5 attributes and added some code to wrap long attributes. Patch applied. Added some error handling to avoid getting exceptions when a tooltip tried to show. 5 is fine, the key thing is that they are limited. Bootstrapping on it now to verify further. (In reply to comment #49) > Patch applied. Added some error handling to avoid getting exceptions when a > tooltip tried to show. 5 is fine, the key thing is that they are limited. > Bootstrapping on it now to verify further. Shoot. I forgot to add "..." when there is more then 5. Also, it would make sense to show similar diffs for outgoing changes. Please note that we have to be *extremely* careful of making any other changes at this point in the game and I'm not sure if it was a good idea to take this one. This patch is inadvertently causing tooltips to fail to appear when they need to go off the bottom of the screen. This was likely not noticeable because the tooltips were not as large. Fix would be appreciated since I'm not sure if Rob or I will have time to investigate. Due to potential for adding regressions major positioning changes, such as bug 189313, will have to wait post 2.0. (In reply to comment #51) > This patch is inadvertently causing tooltips to fail to appear when they > need to go off the bottom of the screen. This was likely not noticeable because > the tooltips were not as large. Fix would be appreciated since I'm not sure if > Rob or I will have time to investigate. I believe this behavior were there before (could be related to changes for multiple monitor support) and it only became more noticeable because tooltip got bigger. Unfortunately I won't have time to look at this this week. > Due to potential for adding regressions > major positioning changes, such as bug 189313, will have to wait post 2.0. I know and that is why I haven't submitted that patch yet. (In reply to comment #52) > I believe this behavior were there before (could be related to changes for > multiple monitor support) and it only became more noticeable because tooltip got > bigger. Unfortunately I won't have time to look at this this week. Yes, that's exactly what I was trying to say. It is triggered by the new functionality but not caused by it. But to the user that's irrelevant. Rob and I will have to look at it. Created attachment 72527 [details]
fixes whitespaces in updated attribute values
Some notifications for jira queries don't look pretty. This patch fixes that by removing whitespaces and handling max line length in field values.
Mik, this is kinda last minute changes, but we need to either fix this or disable notifications on attribute changes until 2.0. I am not sure which option is more prefereable at this point.
Patch applied. Great! Thanks Mik. After 2.0 we may need to review this functionality and handle tooltips for outgoing and conflicting changes... Also, I don't really like to play blameshifting games, but it seems like changes made by the first patch could have changed some positioning and event handling logic. You can compare revisions 1.40 and 1.50 or the TaskListToolTipHandler class, I guess that is the one to blame... So, it all happens from June 18 - 25 (my patches wen't in on 25th) and there wasn't really many dev builds in that time period. Created attachment 72607 [details]
null check
The conflict with the context menu is most probable due to the refactoring I did although I can't determine why this would be happening.I did not intend to change the event handling code that shows the tooltip. I did change the code in hideTooltip() to always invoke tipShell.close(); instead of tipShell.setVisible(false); since the tipShell was always recreated anyways when shown.
Mik, the attached patch adds a missing null check.
Stefefn: just to double-check, this patch has been addressed, right? Yes, it was merged as part of another patch. Are we going to continue tweaking tooltip under this bug (i.e. fix showing info for new and outgoing changed fields)? The original issue had been actually addressed. +1 for closing this issue +1, other enhancements can be raised as new bugs or added to the UI Nits page. Wrong assignee. I'm not totally sure but I think that Steffen had the bulk of the contributions so reassigning to him. Resolved. Actually Eugene did most of the work on this bug. Done. I wouldn't agree with that looking at the number of patches from you and the initial work you've done about layout. (In reply to comment #68) > I wouldn't agree with that looking at the number of patches from you and the > initial work you've done about layout. I assigned bug 193029 to me for the refactoring of the class and the url tweaking. Most of the layout fixes were contributed by you (and Mik). *** Bug 152985 has been marked as a duplicate of this bug. *** |