-
Notifications
You must be signed in to change notification settings - Fork 228
FormText.paint(): don't pass (0,0) to Image on Linux #3089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bundles/org.eclipse.ui.forms/src/org/eclipse/ui/forms/widgets/FormText.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.forms/src/org/eclipse/ui/forms/widgets/FormText.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.forms/src/org/eclipse/ui/forms/widgets/FormText.java
Outdated
Show resolved
Hide resolved
|
So should we
I'm fine with return, at least on Linux it seem to work. However I don't have Mac and I would not like to test on Windows (need more time to setup environment). So if @fedejeanne or @amartya4256 could test on other OS we can do what fits best. |
|
I tested the current PR (calling We can continue this discussion too if you want, but I'd like to have this fixed fast since I'm out of office the next 3 days and this PR fixes the regression. Would you agree, @iloveeclipse and @amartya4256 ? |
|
I tested it out on both windows and linux and I do see no side effect of returning on if the size is 0. However, to avoid any unprecedented issue, I tested out with the following code and it seems the safest to me: |
|
Interestingly, I've noticed while debugging the "bad" size of the FormText is only returned on fullscreen shells. If I minimize the window, no "zero" size is returned. Bug in GTK's implementation of |
Hmm, we have about ~50 tests failing in our product due to this issue, but as far as I can tell none of them are using any fullscreen shells. Not really sure where exactly the (0,0) is coming from. |
|
|
I still don't understand why is it better to try and paint something when the size is I tested the current PR in Mac and I didn't see any side-effects. Feel free to merge as I'll be offline for most of the next 3 days. Thank you all for the input and thank you @iloveeclipse for the prompt fix! |
it is just defensive style - use whatever was working before to stay compatible. |
|
OK, I think we have a consensus. Let's merge. |
Fixes #3087