Community
Participate
Working Groups
Build ID: I20080609-1311 Steps To Reproduce: 1. Open a task editor on a task with lots of comment text 2. Expand the task editor to a horizontal width greater than 550 pixels wide 3. Observe the wasted space on the right side of the editor where comment text is not flowing
Created attachment 105187 [details] screenshot showing unused (wasted) space
fix for this bug must not cause performance issues. See related bug 168557
It is intentional that the text has a fixed width. I find it difficult to follow long lines on the screen when reading comments or descriptions. +1 for adding a sandbox setting to experiment with dynamic resizing of comments
Though individuals may have a preference on layout, the 'eclipse way' with other editors (such as the Java editor, XML editor, Text editor, etc) is to use all available space. +1 for default behaviour to use all available space
I guess I should put my comment into context: If I've invested in a nice big monitor, I want to be able to use it. Failure to make good use of available screen space means more vertical scrolling. This is why most (all?) other editors and views use available horizontal space. If you want your editor to be less wide for any reason, there are lots of options available to you: # you can resize the eclipse window # you can resize the editor space by making adjoining views larger # you can put multiple editors side-by-side # you can make eclipse create a new window
I am aware of that and it was not at all my intention to suggest that this is a bad idea. It is a significant change of the UI though and I believe that there is a benefit to the way the task editor works currently. Making it a sandbox preference would enable us to gather feedback from a larger user base and to get a sense of the performance impact without affecting all users. I am not sure if it is worth discussing if this has to be a sandbox option or not at this point. If there is a patch everybody interested can apply it to their workspace and if the benefit is obvious I am sure it'll make it into cvs soon enough :).
Created attachment 105303 [details] patch that causes comments and description to flow to fill horizontal space
Created attachment 105304 [details] mylyn/context/zip
Created attachment 105305 [details] screenshot showing editor with patch applied
With the patch applied I now get a horizontal scroll bar in every task editor. Is there a way to reduce the width of the comment viewer widgets by a few pixels to avoid that? I have also noticed that I sometimes get whitespace below expanded comments now. I'll attach a screen shot.
Created attachment 105557 [details] white space below comment
(In reply to comment #10) > With the patch applied I now get a horizontal scroll bar in every task editor. > Is there a way to reduce the width of the comment viewer widgets by a few > pixels to avoid that? Sure. Chances are the LayoutManager is not accounting for insets, margins or borders in some way. I'll take a look. > I have also noticed that I sometimes get whitespace below expanded comments > now. I'll attach a screen shot. Hmmm... that's no good. I'll take a look at that, too. Does this happen for all comments or just for specific ones?
I don't always see it. The problem might be that the parent composite does not have its final width set when the editor is constructed. That would cause the SyledText to calculate the wrong height because it calculates it's wrapping based on that width. Take a look at TaskEditorRichTextPart.getEditorWidth(), it has some code approximate the width on editor construction.
thanks for the tip. BTW, if I resize the editor the whitespace issue disappears. That makes me think you're right about it being a problem during initialization.
Created attachment 105660 [details] a new patch solving layout issues this patch is based on the first and solves the following issues: - no horizontal scroll bar unless non-flowing controls require it - no excess whitespace below comments or description - cleaner code in AbstractTaskEditorPage
Created attachment 105661 [details] mylyn/context/zip
I still get the horizontal scroll bar on Linux with the latest patch on open and refresh. I can also not "shrink" a task editor, when I make it less wide I get a horizontal scroll bar as well. I also noticed that the task editor that was restored on startup did not wrap the text in the description, the editor was as wide as the longest sentence in the description. As a minor nit the task editor looks more crowded without the margins for comments. The old code used to set a bit of spacing around each comment: ecLayout.marginBottom = 3; ecLayout.marginLeft = 15;
(In reply to comment #17) > I still get the horizontal scroll bar on Linux with the latest patch on open > and refresh. I can also not "shrink" a task editor, when I make it less wide I > get a horizontal scroll bar as well. I also noticed that the task editor that There are still some items in the editor that require a minimum width (not the comments or description) and these can force the editor to have a horizontal scroll bar. Can you attach a screenshot where you think the horizontal scroll bar should not appear? > was restored on startup did not wrap the text in the description, the editor > was as wide as the longest sentence in the description. If the description has a newline in it then text following the newline will always be on the next line. Is that what you observed? > > As a minor nit the task editor looks more crowded without the margins for > comments. The old code used to set a bit of spacing around each comment: > > ecLayout.marginBottom = 3; > ecLayout.marginLeft = 15; > We can put these margins in if you think they're needed. Personally I like greater information density. Please let me know what you want.
Created attachment 105668 [details] restored editor
(In reply to comment #18) > (In reply to comment #17) > > I still get the horizontal scroll bar on Linux with the latest patch on open > > and refresh. I can also not "shrink" a task editor, when I make it less wide I > > get a horizontal scroll bar as well. > > There are still some items in the editor that require a minimum width (not the > comments or description) and these can force the editor to have a horizontal > scroll bar. Can you attach a screenshot where you think the horizontal scroll > bar should not appear? My mistake, it was a really wide email address in the People section that was causing the scroll bar to appear. > > was restored on startup did not wrap the text in the description, the editor > > was as wide as the longest sentence in the description. > > If the description has a newline in it then text following the newline will > always be on the next line. Is that what you observed? I think it is related to calculating the width of the first editor that is restored before the workbench is visible (see screenshot). > We can put these margins in if you think they're needed. Personally I like > greater information density. Please let me know what you want. On second thought the layout without margins does look more concise. I think I still want the margin on the bottom to separate the comments more but we can keep it for now and get some more mileage and feedback from others.
(In reply to comment #18) > (In reply to comment #17) > > As a minor nit the task editor looks more crowded without the margins for > > comments. The old code used to set a bit of spacing around each comment: > > > > ecLayout.marginBottom = 3; > > ecLayout.marginLeft = 15; > > > > We can put these margins in if you think they're needed. Personally I like > greater information density. Please let me know what you want. Forget my comment about information density: the editor looks way better with the margins as you described. I've updated the layout to support margins.
Created attachment 105673 [details] patch with margins added margins renamed ShowAllLayout -> FillWidthLayout javadocs
(In reply to comment #20) > > We can put these margins in if you think they're needed. Personally I like > > greater information density. Please let me know what you want. > > On second thought the layout without margins does look more concise. I think I > still want the margin on the bottom to separate the comments more but we can > keep it for now and get some more mileage and feedback from others. Oopps! Too bad we're not communicating in real time! Ok, what'll it be -- margins or no margins? Your call.
Created attachment 105675 [details] patch that solves the workspace initialization problem hopefully this is the final cut. It has margins, causes text to wrap and fill available space, and it works around the restored editor issue.
Created attachment 105676 [details] mylyn/context/zip
> Oopps! Too bad we're not communicating in real time! Ok, what'll it be -- > margins or no margins? Your call. It feels like real time ;). I'm fine with either layout. I'm still seeing horizontal scroll bars on Linux when opening tasks that have a vertical scroll bar: 1. Open task 197455 2. Horizontal scroll bar appears (it scrolls about 4 pixels) 3. Resize Eclipse to make scroll bar go away 4. Close and reopen the task The scroll bar reappears. It is fixed when I tweak FillWidthLayout.computeSize() to subtract 25 instead of 15. It would be best if these numbers would be extracted to constants with meaningful names. I see some flickering on open that seems to be caused by the asyncExec() in AbstractTaskEditorPage.reflow(). Is there a way to avoid the reflow does not need to be delayed?
(In reply to comment #26) > I'm still seeing horizontal scroll bars on Linux when opening tasks that have a > vertical scroll bar: > > 1. Open task 197455 > 2. Horizontal scroll bar appears (it scrolls about 4 pixels) > 3. Resize Eclipse to make scroll bar go away > 4. Close and reopen the task > > The scroll bar reappears. It is fixed when I tweak FillWidthLayout.computeSize() > to subtract 25 instead of 15. It would be best if these numbers would be This must be a platform-specific number, since 15 works fine on my Mac. I'm not sure why it's needed, it could be related to the width of a scrollbar. Any thoughts on how to fix this isuse? > extracted to constants with meaningful names. Yes. > > I see some flickering on open that seems to be caused by the asyncExec() in > AbstractTaskEditorPage.reflow(). Is there a way to avoid the reflow does not > need to be delayed? The reflow is only needed if the editor is restored during workbench startup. Any other editor creation doesn't need it at all. Perhaps we could detect that case and live with flicker for those editors only. What do you think?
> This must be a platform-specific number, since 15 works fine on my Mac. I'm not > sure why it's needed, it could be related to the width of a scrollbar. Any > thoughts on how to fix this isuse? Let's pick a reasonable large integer that works on Linux, Mac and Windows. Mylyn does this in a number of places. > > I see some flickering on open that seems to be caused by the asyncExec() in > > AbstractTaskEditorPage.reflow(). Is there a way to avoid the reflow does not > > need to be delayed? > > The reflow is only needed if the editor is restored during workbench startup. > Any other editor creation doesn't need it at all. Perhaps we could detect that > case and live with flicker for those editors only. What do you think? Yes, that would be perfect. Another thing that would be worth investigating is the resizing of the new comment section when opening task editors. It seems that once a all controls are constructed and displayed the form is reflown and the new comment section is assigned its final size causing it to flicker. The reason why the section is handled differently than other sections is the code in AbstractTaskEditorPage.initializePart() which makes it expand vertically: GridDataFactory.fillDefaults().align(SWT.FILL, SWT.FILL).grab(true, true).applyTo(part.getControl()); David, do you have any ideas how this could be avoided? Note that this is entirely unrelated to your patch and present in the current implementation as well.
(In reply to comment #28) > Let's pick a reasonable large integer that works on Linux, Mac and Windows. > Mylyn does this in a number of places. Any chance you could come up with the smallest possible number for Windows and Linux? I don't have access to those platforms. > > > > I see some flickering on open that seems to be caused by the asyncExec() in > > > AbstractTaskEditorPage.reflow(). Is there a way to avoid the reflow does not > > > need to be delayed? > > > > The reflow is only needed if the editor is restored during workbench startup. > > Any other editor creation doesn't need it at all. Perhaps we could detect > that > > case and live with flicker for those editors only. What do you think? > > Yes, that would be perfect. Okay.... do you know of a reliable way of detecting this scenario? > > Another thing that would be worth investigating is the resizing of the new > comment section when opening task editors. It seems that once a all controls are > constructed and displayed the form is reflown and the new comment section is > assigned its final size causing it to flicker. > > The reason why the section is handled differently than other sections is the > code in AbstractTaskEditorPage.initializePart() which makes it expand > vertically: > > GridDataFactory.fillDefaults().align(SWT.FILL, SWT.FILL).grab(true, > true).applyTo(part.getControl()); > > David, do you have any ideas how this could be avoided? Note that this is > entirely unrelated to your patch and present in the current implementation as > well. There are two approaches that might work that I'm familiar with. One is to add a resize listener on the control's parent, and whenever the control is resized set the width hint on the control. Very much a hack, but relatively easy. The other way is to use a layout manager similar to FillWidthLayout.
Created attachment 105771 [details] new patch addresses workspace startup issue issues addressed in this patch: * editors created on workspace startup reflow(), other editors do not (eliminating unnecessary flicker) * hard-coded right margin for width hint is now OS-dependant
What is left to do here? Steffen?
I'll apply the latest patch to my base workspace and run on it for a bit.
I noticed a couple things: * Editor min size is lager (horizontal scroll bar visible until editor resized quite wide) * Last few characters along rh margin of description/comments not visible in some scenarios requiring manual sizing of editor to make visible
(In reply to comment #33) > I noticed a couple things: > * Editor min size is lager (horizontal scroll bar visible until editor resized > quite wide) > * Last few characters along rh margin of description/comments not visible in > some scenarios requiring manual sizing of editor to make visible > I assumed that this was due to other elements (like email, attributes) pushing out the width of the editor. If that's not the case then it'll need fixing. We may need to resort to SWTSpy to figure it out.
> I assumed that this was due to other elements (like email, attributes) pushing > out the width of the editor. If that's not the case then it'll need fixing. We > may need to resort to SWTSpy to figure it out. The (Bugzilla) task editor should now shrink to 550 px of width. Email addresses in the people section should get cut off when they are too wide. With the latest version of the patch I ran into a scenario again where I was not able to shrink the editor. It would retain it's width and get a horizontal scroll bar. The scroll bar disappeared when I closed the task editor and reopened it.
(In reply to comment #35) > The (Bugzilla) task editor should now shrink to 550 px of width. Email > addresses in the people section should get cut off when they are too wide. Do email addresses (or their container) have a maximum width setting? If not they'll just flow to occupy as much space as they can get. > With the latest version of the patch I ran into a scenario again where I was > not able to shrink the editor. It would retain it's width and get a horizontal > scroll bar. The scroll bar disappeared when I closed the task editor and > reopened it. Interesting. When I have time I'll take a look. I've noticed http://wiki.eclipse.org/PDE/Incubator/Picasso which looks like it could be very helpful. Remember that with this patch the text of comments/description should occupy the width determined by the widest component on the page.
I've made some changes to how the layout works that I believe resolves the remaining issues. The key to the solution is to use the editor CTabFolder parent as the 'advisor' to the size of the client area. That seems to be the most reliable way to determine the size of the editor outside of the scrolling area.
Created attachment 106983 [details] new patch that updates layout algorithm
Created attachment 106984 [details] screenshot showing new layout algorithm in action painted with picasso notice that the attributes area with margins is determining the editor width. Any less wide than this and it gets a horizontal scrollbar.
Created attachment 107152 [details] cut off text and horizontal scroll bar (bug 239087)
(In reply to comment #40) > Created an attachment (id=107152) [details] > cut off text and horizontal scroll bar (bug 239087) > Steffen, thanks for sticking with me on this one. As it turns out, getting it right is very hard. I'll iterate on this one again.
Created attachment 107213 [details] patch that fixes word cut-off issue turns out that FillWidthLayout was not correctly applying margins when performing the layout
Steffen, this one's ready for you!
Thanks David. I'll keep working with the patch and have scheduled it for review early in the 3.1 release cycle.
Created attachment 110406 [details] new patch that fixes horizontal scrollbar issue I've discovered why the horizontal scroll bar would present itself. Basically without a reflow() the height of the text control cannot be increased or decreased. So we have two scenarios: # when the editor is resized larger the source viewer would either be too high for the wrapped text and thus have excess whitespace # when the editor is resized smaller the source viewer would be too short and thus force the wrapped text to use horizontal space, requiring the horizontal scrollbar To solve this issue a reflow() is performed when the editor is resized. This allows the height of the text control to become smaller or larger as required, thus eliminating unused whitespace and the horizontal scrollbar.
Created attachment 110407 [details] mylyn/context/zip
Created attachment 113955 [details] updated patch
I have committed the updated page. The only modification I made is to hide getLayoutAdvisor() in an internal class instead of adding new API. There are three concerns left that I would like to address before closing the bug: * The listener in AbstractTaskEditorPage.createPartControl() causes an extraneous reflow on first open of a task editor. * The listener in AbstractTaskEditorPage.createPartControl() slows resizing of the editor noticeably. Would it be feasible to delay the reflow until resizing has finished? In the past we have used UI jobs for that but it would be nicer if SWT had an event for that similar to the post selection events provided by JFace viewers. * The layout is too heigh when making the editor smaller than its minimum width (see screenshot).
Created attachment 113956 [details] white space below description of this bug
(In reply to comment #48) > * The listener in AbstractTaskEditorPage.createPartControl() slows resizing of > the editor noticeably. Would it be feasible to delay the reflow until resizing > has finished? How about using some kind of 'nagle' delay with a timer?
(In reply to comment #48) > I have committed the updated page. Thanks for getting this one in there... it really makes a difference!
> > * The listener in AbstractTaskEditorPage.createPartControl() slows resizing of > > the editor noticeably. Would it be feasible to delay the reflow until resizing > > has finished? > > How about using some kind of 'nagle' delay with a timer? Yes, we use a heuristic for task list refresh where we queue up multiple refreshes to avoid constantly keeping the UI busy during synchronizations. The implementation is in DelayedRefreshJob.
Created attachment 116251 [details] optimizations
The patch improves performance by a magnitude when the editor reflows by not recalculating unchanged values. FillWidthLayout now breaks the contract of Layout by not flushing it's cache. I have made it package protected to avoid reuse of the class outside of it's current use-case. I have also tweaked the margin to avoid blank lines (comment 49). David, are you planning to work on delaying reflows when the editor is resized?
(In reply to comment #54) > David, are you planning to work on delaying reflows when the editor is resized? I'll take a look
Created attachment 116309 [details] patch that delays reflow on resize the attached patch greatly reduces the number of reflows on editor resize by applying a 'nagle' algorithm
Thanks David, I have applied the patch with a minor modification: the listener checks if the form is disposed before calling reflow in case the editor was closed in the meantime. Thanks for your great work on this bug! Please feel free to resolve if we are done here.
(In reply to comment #57) > Thanks David, I have applied the patch with a minor modification: the listener > checks if the form is disposed before calling reflow in case the editor was > closed in the meantime. Oops... I'd forgotten to do that. Thanks. > Thanks for your great work on this bug! Thanks! It took awhile, but the wait was worth it. Thanks for collaborating with me on this one. > Please feel free to resolve if we are done here. Done.
