Conversation
|
@rfm added this code few months ago, so he should have the say on whether we drop this again. Either way is fine with me. |
|
As I recall, the current code was written to make the tops of menus available so allow them to be moved and the first menu item selected reliably, and achieved that objective. Before the last lot of changes, the top part of our menus would be hidden behind the top bar controlled by many window managers. However, it's hardly surprising if there are bugs: it was a quick/simple fix. The problem I have here is with this sentence:
The current model is precisely because my understanding was that an OpenStep/AppKit display is the usable display, because apps expect to be able to draw anywhere on screen, and that when OpenStep was designed for display postscript, there were no window managers, no struts, and no difference between the physical screen and the usable screen. I'm happy to be educated otherwise, but what are the AppKit/gnustep-gui methods used by apps to to query panels and struts, and where can I see examples of Apple and GNUstep app source using those methods to position windows/menus? |
|
Hi @rfm, thanks for your quick response. Obviously my experience with GNUstep is not as deep as yours, but here are my thoughts: AppKit distinguishes between the physical display ( I agree the current work-area code was a reasonable quick fix to keep menus reachable below the WM's top bar. My concern is narrower: it is internally inconsistent. The screen frame ( That inconsistency bites either way:
I'd suggest we fix the consistency (whichever direction) rather than leave the flip and the frame disagreeing. The alternative direction (keep the work-area frame but flip against the same height) has the same side effect of moving top-positioned windows, while keeping For the Gershwin |
|
I wouldn't claim to be deeply experienced with the gui/backend as I don't write gui apps and hardly ever touch it, and I don't actually know why I'm not seeing any issues if _OSFrameToXFrame if getting things wrong, but I'm perfectly prepared to accept there is a bug there. I also don't understand why you want to draw at the top of the physical screen when the window manager is going to ensure that its own topbar, another window outside of GNUstep control, will hide anything you draw. My understanding is that there are at least three areas to consider: Now, what I did with _NET_WORKAREA was to try to conflate the physical area with the usable area, on the theory that if the GNUstep app can never display outside the usable area, then the usable area corresponds to the NSScreen rect as far as the GNUstep app is concerned. Maybe that's wrong, and the NSScreen rect should correspond to the physical display, but if so, the GUI drawing code needs to understand the distinction between the physical screen rectangle and the usable screen rectangle somehow, so that it doesn't try to position the main menu, menubar etc outside of the usable part of the display (which is what was previously happening). So unless I'm missing something, I think we can either: Drop the workarea adjustment so that the screen rectangle is the physical size as reported by X, but at the same time keep the workarea code to find the usable screen rectangle, pass the usable rectangle back to the GUI. Add a new internal method to find the usable rectangle, adjust the -visibleFrame method to incorporate the usable rectangle, and adjust the menu code to use the usable rectangle information to ensure that main menus/menubars are appropriately constrained to be within the usable area of the screen. or We can have the NSScreen rectangle report the usable (to the GNUstep app) area of the physical screen, and fix any errors in the OpenStep<->X coordinate mapping resulting from that. Does that make sense? |
|
I think this is a design/policy thing that Fred should decide on: I can see arguments either for both ways (I was lazy and chose the option that looked like least work). However, as far as I can see this PR on its own just breaks placement of menus and windows when used with the current main window managers: it would need to be coupled with all the changes to get menu placement and the -visibleFrame calculation correct. |
|
Hi @rfm, @fredkiefer, As I understand it, the underlying issue is that There seem to be two consistent ways to fix this: Option (a):
|
|
I think to do this we would need to:
The main menu in turn could ask NSScreen for visibleFrame, and position itself by its own height above it, because it knows that vusibleFrame has already taken it's height into account. When changing themes we would need to be careful to change main menu height first, then get NSScreen to update, then reposition menus if necessary. |
_NET_WORKAREA holds the area a window manager leaves usable, in X coordinates, where y grows downwards from the top of the screen. A screen frame is in OpenStep coordinates, where y grows upwards, so the rectangle has to be flipped. The origin was set to zero instead. The origin says which end of the screen the reserved rows are at. A panel at the top and a panel at the bottom reserve the same number of rows and report the same height, so zeroing the origin gives both the same frame. That frame is right for a panel at the top and wrong by the height of the panel for one at the bottom, where a window placed at the bottom of the screen was drawn under the panel and the rows at the top could not be reached. Measured on a 1920x1080 screen with 40 rows reserved at the bottom: a 50 pixel bar placed at the bottom of the screen was mapped at X row 1030, over a panel occupying rows 1040 to 1079, and is now mapped at row 990. A bar placed at the top was mapped at row 40 and is now at row 0.
_NET_WORKAREA refers to the composite display formed by all the monitors, so it is only used as a screen frame when there is a single monitor. That test read monitorsCount while it still held screen_res->noutput, the number of RandR outputs. monitorsCount holds the number of monitors only after the outputs have been walked, since an output the display is not using has no CRTC and is not a monitor. A driver reports an output for every connector it supports, so the count matched only on a server that reports exactly one. Measured with the Xorg dummy driver, which reports 16 outputs: with a single monitor and a work area 40 rows shorter than the screen, the screen frame was the full 1920x1080 and is now 1920x1040 at y 40, which is the frame Xvfb already gave through its single output.
A panel at the top of the screen and a panel at the bottom reserve the same number of rows, so they report the same work area height and differ only in the origin. They have to give different screen frames. The test publishes _NET_WORKAREA on the root window and reads the frame back through boundsForScreen:. It creates the display server directly, since nothing here draws, and skips when there is no display or when more than one monitor is present, which is the case the property is not used for. Tests/x11 now links the gui library, which is where GSDisplayServer lives.
|
Some measurements on this, all with a single monitor. The work area is only applied when the server reports exactly one RandR Where the override does apply, the shift is real. With 40 rows reserved, a 60 Separately, _workAreas sets origin.y to zero instead of flipping it, so a #226 corrects both of those inside the current model. It does not settle |
…igin # Conflicts: # Tests/x11/GNUmakefile.preamble
XGServerWindow.m keeps the two comments master already had. The header of Tests/x11/workarea.m states what the test covers and no more.
|
Hi @rfm, I would like to address your concerns fully. In order to do so. I need to reproduce the issue you see with my proposal. Can you give me step-by-step instructions that highlight the issue? Thanks! |
Switch back to master and rebuild back. re-launch the app. You will see the menu correctly positioned immediately below the topbar. |
|
@rfm, I think the reproduction is useful because it exposes an important distinction that the current code is mixing together. I would separate four things rather than treating 1.
On X11, the backend gets monitor geometry from RandR. On Windows, the corresponding per-monitor distinction is That distinction matters because the monitor geometry is also part of the coordinate-system definition. 2. The X11 WM may reserve space for panels/docks using So
It does not answer:
There is an important multi-monitor qualification here too. So the backend should not simply replace every monitor's 3. The current X11 bug is the mixing of those coordinate spaces The problematic operation in the backend is effectively: monitors[0].frame = workArea;That makes But the X/OpenStep coordinate conversion still performs its Y inversion against the full X screen height. The resulting problem can be made concrete. Suppose the monitor is 1920×1080 and the WM reserves a 40-pixel panel at the top. The X11 work area is then: If that rectangle is used as But So the point which was supposed to represent the top of the reported screen frame ends up at X11 y=1040 — i.e. 40 pixels below the physical top of the monitor. The problem is therefore not that the work area itself has the wrong origin. The problem is that That is the frame/coordinate-conversion mismatch which produces the observed displacement. 4. This is where I think the GUI side needs to be explicit about the different reservations. The desired model is not simply: because the menu reservation must not be subtracted twice if it is already represented by the WM work area. Instead:
The menu bar itself is a separate placement question. We should place the menu bar in the strip reserved by GNUstep at the top of the monitor — or at the top of the WM work area when the WM has already reserved the area above it — and then make That keeps the menu placement and So I think the clean division for this change would be:
That also explains the Ink.app reproduction. If PR #217 changes I therefore agree with the conclusion that PR #217 should not be considered complete on its own. But I don't think the example is evidence that That gives us a consistent model: with the important proviso that a reservation must only be accounted for once. I'd be happy to hear your thoughts. |
|
I think that matches the conclusion I reached a few weeks ago, but with a bit more detail. |
|
To be clear, I do agree with that latest analysis but I don't think it moves us any further on. The way to progress is to actually make that whole change, which requires changing both back and guí together, a PR for each which can be merged at the same time. The two PRs connect using a new method for gui to ask back which part of each screen is available for menus and which part for normal windows. |
|
NB. In the common case where gnustep menus are not integrated with the window manager, screen availability for menus and normal windows would be the same. I guess this would depend on the interface style selected, which is known to the GUI, so perhaps two separate methods would make sense, one to let the GUI ask about where it can place normal windows, and one to ask where the window manager allows the app main menu to be placed. |
So we agree conceptually. It's just that the "second part" of the solution is missing. I will see what I can do. |
|
Another data point for the output count issue @DTW-Thalion measured on the Xorg dummy driver: it also happens on ordinary laptops. On a laptop with a single built-in panel, So today whether the work area is used depends on how many connectors the graphics hardware exposes, not on how many monitors are attached. Counting only outputs that have a CRTC before the single monitor test (as #226 does) makes the laptop behave like Xvfb. |
|
Thinking about where #226 leaves this PR. #226 fixes the two concrete defects in the current model: the work area origin is now flipped against the full screen height instead of being zeroed, and the single monitor test counts monitors instead of outputs. With the origin flipped, What #226 does not address is the design question discussed above:
So I don't think this PR is redundant, but it should not be merged as it is. I see two ways forward:
I lean towards 2 on top of #226, so that #226 can be merged first and the redesign stays a separate, reviewable step. Opinions welcome, @rfm @fredkiefer @DTW-Thalion. |
|
I agree: you have convinced me that the screen frame is meant to reflect the physical display |
The X11 backend had no way to pass on the area a window manager leaves to application windows, and it used that area as the screen frame instead whenever there was a single monitor. A frame that is not the whole screen disagrees with the coordinate flip the backend does for windows, so a window asked for at the top of the screen landed lower down by whatever the panels reserve, while an application that wanted to avoid the Dock had nothing to ask. The screen frame is now always the whole monitor, the work area is read from _NET_WORKAREA with its origin flipped into screen coordinates rather than clamped to zero, and -workAreaForScreen: answers it; the other backends answer their full screen, which changes nothing for them. Together with the libs-gui side this is what -[NSScreen visibleFrame] returns. This replaces screen-frame-not-workarea.patch, which took the work area out of the frame but stopped there, and which cannot be applied beside this one. Upstream has the first half open as gnustep/libs-back#217 and the origin flip as #226; the accessor itself is not upstream yet.
The frame of a screen was replaced by _NET_WORKAREA whenever there was a single monitor, so NSScreen described what the window manager leaves over rather than the display. An application could not learn the geometry of the monitor, and one that reserves space itself, a menu bar or a dock, could not place itself in the strip it had just reserved, since that strip lies outside every frame it can see. The frame is the monitor again, the work area is kept beside it, and -workAreaForScreen: answers it. The other servers answer their whole screen, which is what they reserve nothing of. This is the backend half of the pair discussed in gnustep#217; the gui half, which builds -[NSScreen visibleFrame] from the new method, is gnustep/libs-gui#953. It sits on top of gnustep#226, whose flip of the _NET_WORKAREA origin and count of monitors rather than outputs it needs and does not repeat.
613826f to
a238924
Compare
# Conflicts: # ChangeLog
The work area test asserted that _NET_WORKAREA shortens the screen frame, which is what this branch stops it from doing. It now publishes the same two work areas and checks that the backend reports them, origin and all, while the frame stays the whole screen whichever edge is reserved.
2037b0f to
08236d7
Compare
Reworked along the lines agreed above, as the backend half of the pair, and rebased on top of #226 so that its flip of the
_NET_WORKAREAorigin and its count of monitors rather than outputs are not repeated here. The commits of #226 are part of this branch until it is merged; what this PR adds on top of them is one commit.The frame of a screen was replaced by
_NET_WORKAREAwhenever there was a single monitor, soNSScreendescribed what the window manager leaves over rather than the display. An application could not learn the geometry of the monitor, and one that reserves space itself, a menu bar or a dock, could not place itself in the strip it had just reserved, since that strip lies outside every frame it can see.The frame is the monitor again, the work area is kept beside it, and
-workAreaForScreen:answers it. The headless, wayland and win32 servers answer their whole screen, which is what they reserve nothing of.Measured on an 800x600 screen with a 22 pixel menu bar at the top and a dock at the bottom,
_NET_WORKAREA = 0, 22, 800, 450:With the gui half, gnustep/libs-gui#953, on top of each of them:
The 428 on the left is the menu bar counted twice: the work area already excludes the strip the menu bar reserved, and
-visibleFramesubtracts its height again. #953 takes off only as much of the menu bar as the work area has not already been reduced by, which is the "a reservation must only be accounted for once" point from the discussion above.I could not run
Tests/x11on this machine: it wants libXmu, which is not installed here, and eleven of its tools fail to link for that reason before any of them runs. The backend itself builds clean, and the change on top of #226 is confined to-screenListand the new method.cc @rfm @fredkiefer @DTW-Thalion @pkgdemon