Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 361147 - support scrolling and focus in review comment part
Summary: support scrolling and focus in review comment part
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: 0.8   Edit
Hardware: PC Linux
: P3 enhancement (vote)
Target Milestone: 0.9   Edit
Assignee: Steffen Pingel CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2011-10-17 10:30 EDT by Robert Munteanu CLA
Modified: 2012-01-30 09:39 EST (History)
0 users

See Also:


Attachments
Comments cut off (48.31 KB, image/png)
2011-10-17 10:31 EDT, Robert Munteanu CLA
no flags Details
mylyn/context/zip (6.51 KB, application/octet-stream)
2011-10-19 08:47 EDT, Steffen Pingel CLA
no flags Details
Draft patch (2.85 KB, patch)
2011-10-19 10:22 EDT, Robert Munteanu CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Munteanu CLA 2011-10-17 10:30:51 EDT
When opening the review comments widget in the compare editor, the resulting widget is cut off at the bottom, and is not selectable . Pressing 'F2' closes the widget. This was verified running the latest code from the e_3_7_m_3_6_x branch.

Running Eclipse 3.7.1 on Gnome/GTK 2.22.1 , OpenSUSE 11.4 x86_64.
Comment 1 Robert Munteanu CLA 2011-10-17 10:31:56 EDT
Created attachment 205334 [details]
Comments cut off

The fifth comment is cut off and there are a total of 7 comments for that topic. In this installation, the standard tooltips used by e.g. JDT work as expected.
Comment 2 Steffen Pingel CLA 2011-10-18 03:03:56 EDT
That is indeed a problem. Can you suggest a solution?
Comment 3 Robert Munteanu CLA 2011-10-18 03:25:42 EDT
I think that behaving the way the JDT widgets behave would be perfect:

* have a predefined size - the current width is fine, the height needs to be fixed
* enable scrolling if the content height is larger that the defined widget size
* have true focus when pressing F2
Comment 4 Steffen Pingel CLA 2011-10-19 05:52:13 EDT
Thanks! Please let me know if you are interested in working on this.
Comment 5 Robert Munteanu CLA 2011-10-19 05:55:02 EDT
I would be interested, but on the other hand I have no idea how to approach this - any pointers would be useful.
Comment 6 Steffen Pingel CLA 2011-10-19 08:47:33 EDT
Looking at CommentPopupDialog comment parts appear to be embedded in a scrolledComposite already. Maybe it's just a matter of setting the proper size hints. There is similar code in SectionComposite that is used in search pages and has working scrolling support. It could be worth taking a look at as an example. I hope that helps a little bit.
Comment 7 Steffen Pingel CLA 2011-10-19 08:47:35 EDT
Created attachment 205519 [details]
mylyn/context/zip
Comment 8 Robert Munteanu CLA 2011-10-19 09:28:39 EDT
Thanks, I'll see what I can do.
Comment 9 Robert Munteanu CLA 2011-10-19 10:22:57 EDT
Created attachment 205532 [details]
Draft patch

I've set up this patch, which allows scrolling ; focusing is still problematic. On both 3.5 and 3.7 target platforms I am able to focus, given that I wait for a couple of seconds with the mouse over the 'review' annotation and them move the mouse over the dialog.

I _think_ that the root cause is that the @CommentInformationControl.setFocus@ method is never called, therefore the CommentPopupDialog is never focused, which leads to the size not being recomputed. This patch takes us closer to having proper focus + scrolling, but I'd like to find out why setFocus was never called in the first place.

I'm not sure exactly how to debug this. Steffen, any ideas?
Comment 10 Steffen Pingel CLA 2011-10-19 12:44:38 EDT
I'll try to find some time at the end of the week to look into this further. I am not sure what else is needed to make the focusing work.
Comment 11 Robert Munteanu CLA 2011-10-21 10:51:44 EDT
Would it be acceptable to target this localised fix for 3.6.3 and leave the issue open for a proper fix in 3.7 ? It would be an advantage to be able to have scrolling , as comments are right now simply cut off with no indicator that there is more information.
Comment 12 Steffen Pingel CLA 2011-10-21 11:37:00 EDT
(In reply to comment #11)
> Would it be acceptable to target this localised fix for 3.6.3 and leave the
> issue open for a proper fix in 3.7 ? It would be an advantage to be able to have
> scrolling , as comments are right now simply cut off with no indicator that
> there is more information.

Sure, we can try. I can't promise that I'll have time to get to it though since we have scheduled 3.6.3 for mid next week already.
Comment 13 Robert Munteanu CLA 2011-10-21 15:17:09 EDT
(In reply to comment #12)
> (In reply to comment #11)
> > Would it be acceptable to target this localised fix for 3.6.3 and leave the
> > issue open for a proper fix in 3.7 ? It would be an advantage to be able to
> have
> > scrolling , as comments are right now simply cut off with no indicator that
> > there is more information.
> 
> Sure, we can try. I can't promise that I'll have time to get to it though since
> we have scheduled 3.6.3 for mid next week already.

Thanks, that sounds reasonable. I'll do some more tests by Monday on non-Linux platforms too see if this patch introduces any inconsistencies.
Comment 14 Robert Munteanu CLA 2011-10-24 03:35:03 EDT
Applying the fix on top of e_3_7_m_3_6_x does not enable scrolling on Windows 7 . At least I can confirm that the same problems as on Linux/GTK appear. I suggest holding off this patch until we have a better fix.
Comment 15 Steffen Pingel CLA 2012-01-23 10:23:48 EST
Moving to 0.9 backlog since that release is planned shortly after the maintenance release. That  will give us a little bit more time to implement this.
Comment 16 Steffen Pingel CLA 2012-01-30 09:39:14 EST
I have committed 8131afb54cc52320662111ac9eb45b86d8ea4f00 to fix the scroll bar. I didn't find a fix for focusing and hence have removed the Focus label for now. We'll need to revisit that later.