Bugzilla – Full Text Bug Listing
| Summary: | [ModelEditor] TrimWindow Bounds set to -2147483648 | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Lars Vogel <Lars.Vogel> | ||||
| Component: | Tools | Assignee: | Lars Vogel <Lars.Vogel> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | bsd, Lars.Vogel, nobody, tom.schindl | ||||
| Version: | unspecified | ||||||
| Target Milestone: | 0.16 | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
|
Description
Lars Vogel
See bug 350226: unfortunately negative values are possible with certain monitor configurations. Some outlandish value seems the only possibility, short of introducing a controlling boolean (e.g., definesPosition). +Brian: While possible I don't think they would be the default ones. I think 0 0 for x and y are good default values if you create a new TrimWindow. I think the better approach would be to have the model editor simply show blank if the value is the default. Learned that -2147483648 is actually a default value in Windows. Closing this bug as won't fix. I'm reopening -- I think the model editor shoulda show these values if they're the default, but instead grey the field names (or something). @Brian: Gray would indicate that the field is not input ready and I think that would confuse the user. -1 from me for gray fields. I think 0 0 is a good compromise to this. I don't think any monitor configuration or scenario in which 0 0 can cause problems. After all it's just a default value and it should be modified if needed. A greyed out 0,0 is what I was thinking. It could be set using Text.setMessage(), and will just be blank on platforms without #setMessage() support. Lars: Integer.MIN_VALUE isn't a default but a marker for 'no value'. Unfortunately 0,0 and negative numbers do appear in practice with maximized windows and multi-monitor setups (bug 350226 and bug 358627). @Sopot: I think Windows behaves special if you use -2147483648. AFAIK it uses 0, 0 is there is not another window on this position but start moving it a bit if that space is already used. @Brian: I was told be a Window MFC developer that -21.... is actually used by MS Windows. I have no way to check if that is true. If it is only a marker for 0, 0 then we should actually use 0,0. I guess this is up to the renderers to interpret the values. Moreover suggesting a value linked to the OS is worse. Yesterday I worked with the JavaFX renderers and spent half an hour finding out why the window didn't show up. Changed it to 0,0 and it worked fine. As it turned out my window was being displayed some -2billion pixels away. Also forgive me but I still don't quite understand what we are supposed to convey with the grayed out property. You initially see 0,0; you run; you see it in the TL corner; you don't like it; you change it. We're not hardcoding anything, just proposing an initial value in the editor. +1 for an initial value of 0 without graying it out. We should allow to enter -21... if Windows uses this values, we should allow it. (In reply to comment #10) > @Brian: I was told be a Window MFC developer that -21.... is actually used > by MS Windows. I have no way to check if that is true. It's not used that way within E4. That value was only chosen to work around how EMF implements its #eIsSet(). If you look at WBWRenderer#createWidget(...), it uses eIsSet for each of X/Y/height/width so that we only override the OS-provided defaults if an *actual value was set by the developer*. The EMF eIsSet implementation eventually compares the value with the default. By using an outlandish -2147483648 as the "default" value we ensure no possibility of confusion. This value should be safe for our lifetimes -- even at 1200dpi that would be 4500km. Basically the tooling should do an eIsSet() check as per WBWRenderer and show nothing if true; it should also properly handle deleting the text field to unset the value. (In reply to comment #14) > Basically the tooling should do an eIsSet() check as per WBWRenderer and > show nothing if true; it should also properly handle deleting the text field > to unset the value. Unfortunately I have no idea how to incorporate this into the databindings! Why not showing DEFAULT when we see -21.... ? If someone changed the value and wants back it is easier to remember. It might be a good idea to have a context menu so that one can easily reset it back to DEFAULT I have to say that I hate this -..... - if there's a problem with 0/0 its the job of the renderer to fix this. As I said in earlier comments this should be set to 0,0 in my opinion too. Stealing the bug from Brian to add it to my fix queue, supposing he has not very strong feelings about this. +1 for 0, 0 Remember that 0,0 is a perfectly valid position, as are negative numbers, as occurs when using multiple monitors. See the bug references in comment 8. Good luck! Oh, one other thing: we can't hard-code 0,0 or some other default as an unspecified position allows the OS/WS to position the shell as it sees fit. This is important functionality that must be retained. +1 for Brians comment of retaining the functionality. IMHO setting 0 as default is fine as long as the user can still change back to the OS constants. (In reply to Lars Vogel from comment #22) > +1 for Brians comment of retaining the functionality. IMHO setting 0 as > default is fine as long as the user can still change back to the OS > constants. Not sure what's the consensus here. Do you people agree on 0,0 or not? > Do you people agree on 0,0 or not?
If I understood Brian correctly he is ok with 0 as long as we allow the -2147483648 to be entered by the user. @Brian please feel free to correct me.
(In reply to Lars Vogel from comment #24) > > Do you people agree on 0,0 or not? > > If I understood Brian correctly he is ok with 0 as long as we allow the > -2147483648 to be entered by the user. @Brian please feel free to correct me. The exact opposite Lars :-) I think the real problem here is that "x", "y", "height", and "width" are not marked as being unsettable in the ecore. I'm not sure if we can make these attributes default to being unset within the .ecore, without modifying the WindowImpl. > I think the real problem here is that "x", "y", "height", and "width" are
> not marked as being unsettable in the ecore. I'm not sure if we can make
> these attributes default to being unset within the .ecore, without modifying
> the WindowImpl.
Sounds like we talking about different things. I suggest to set the values to 0 during the creation of TrimWindow element via the e4 tools. This would not need any chance in the ecore model.
I'd have thought the e4 tools should create the same model as if it were created programmatically through the EMF model. And there is also the problem when opening an existing model. (In reply to Brian de Alwis from comment #27) > I'd have thought the e4 tools should create the same model as if it were > created programmatically through the EMF model. We don't do this now, see E4NewProjectWizard mainWindow.setWidth(500); mainWindow.setHeight(400); Sopot, I have the fix in my workspace, I push it to Gerrit (In reply to Brian de Alwis from comment #27) > I'd have thought the e4 tools should create the same model as if it were > created programmatically through the EMF model. Sorry, I need to more precises. E4NewProjectWizard sets values and I think we should change it here. For the right-mouse click, add child -> TrimmedWindows I agree with you, we would not modify the values coming from the EMF model. That is the patch I have pending. (In reply to Lars Vogel from comment #28) > We don't do this now, see E4NewProjectWizard > > mainWindow.setWidth(500); > mainWindow.setHeight(400); But you're not setting the X and Y :-) The real issue here (IMHO) is that the model editor should show blank or something for fields that aren't set. I don't know how to do that with EMF databinding though. A custom update strategy, I guess? > But you're not setting the X and Y :-) Yes, that is the suggested change. The code from above already exists. Suggested change in Gerrit: https://git.eclipse.org/r/17467 Brian pointed in the Gerrit review to https://bugs.eclipse.org/bugs/attachment.cgi?id=203006&action=diff in which he uses "-1" as default values for weight and height. I adjusted the patch, I'm ok with -1 looks much better than -2147483648. Lars, (-1,-1) is a valid combination for multi-monitor setups. The use of -2147483648 is to act as a sentinel to indicate that a value has not been set. It shouldn't be shown in the model editor but should instead show a blank or something to indicate that there is no value. Ok, Brian is strong in this opinion and he has good points. My original request in this bug report was to set the x and y values explicit to something else than -2147483648. As we have no plans to do this, I close this bug again as WONTFIX. Brian, if you want to hide / grey out the fields please open a new bug report for that. How cool is Brian? He provides a patch for this suggestion. https://git.eclipse.org/r/#/c/17499/ Created attachment 236619 [details] Patch to change Window #x, #y, #height, and #width to be unsettable I've pushed up a change to gerrit that uses a new custom UpdateValueStrategy to handle blank fields to represent these "unset" values. This change requires using a patched UIElements.ecore (this attachment) to make x, y, height, and width be unsettable; it turns out this requirement could be dropped, but it seems to be a worthy change. https://git.eclipse.org/r/17499 THe implementation is a bit gross, but EMFEditValueProperty and EMFValueProperty don't actually check or handle the case where the retrieved value is unset. And it's not clear to me (yet) how to provide a custom EMFEditValueProperty decorator. We also need to pass around SetCommand.UNSET_VALUE to do the right thing when a field is changed to blank. Tom: does this look like the right approach? Revised my approach slightly: since our x, y, width, and height have nonsensical default values, we don't need to ensure the attributes are marked as unsettable. Otherwise we'd have to regen the UI model. Committed as: http://git.eclipse.org/c/e4/org.eclipse.e4.tools.git/commit/?id=e606d467ead433a25ad188cd059c8a5d8611c806 |