dolphinsmalltalk / Dolphin

Dolphin Smalltalk Core Image
MIT License
301 stars 58 forks source link

Dialogs always open on the primary display rather than the active display (worked in D6.1B2) #1116

Closed cDemersMitSci closed 3 years ago

cDemersMitSci commented 3 years ago

Describe the bug When a Dialog is opened from a Shell it is centered on the primary monitor even when the parent Shell is on a secondary monitor.

To Reproduce Steps to reproduce the behavior in Dolphin version 7.1.20 :

  1. Open a new Class Hierarchy Browser.
  2. Drag the CHB to a secondary monitor.
  3. Right click on a Class in the Class List.
  4. Execute the Find... context menu command.
  5. The Find a Class Prompt Dialog will open on the primary monitor rather than the secondary monitor where the parent CHB was.

Expected behavior In Dolphin 6.1 Beta 2 Dialogs are always opened on the parent shell monitor. This seems to be a regression in behavior going back to at least a release with a file version of 2016.7.0.50, and possibly before that release.

Please complete the following information):

Additional context I traced some of the view creation and position methods in working and non-working images:

D6.1 Beta 2 : Dialog should (and does) open on left (display 1) montitor. 00000000 [6076] M:ShellView>>createAt:extent: A1: 1399@10 A2: 305@165
00000001 [6076] M:View>>createAt:extent: A1: 1399@10 A2: 305@165 00000002 [6076] M:DialogView>>basicCreateAt:extent: A1: 1399@10 A2: 305@165
... 00000018 [6076] M:View>>centerExtent:within: A1: 305@165 A2: a DesktopView(16r10010, 'Desktop')
00000019 [6076] M:View>>centerExtent:within: A1: 305@165 A2: a DesktopView(16r10010, 'Desktop')
00000020 [6076] M:View>>position: A1: 687@442

D6.1 Beta 2 : Dialog should (and does) open on right (display 2) montitor. 00000021 [6076] M:ShellView>>createAt:extent: A1: 1399@10 A2: 305@165
00000022 [6076] M:View>>createAt:extent: A1: 1399@10 A2: 305@165 00000023 [6076] M:DialogView>>basicCreateAt:extent: A1: 1399@10 A2: 305@165
... 00000039 [6076] M:View>>centerExtent:within: A1: 305@165 A2: a DesktopView(16r10010, 'Desktop')
00000040 [6076] M:View>>centerExtent:within: A1: 305@165 A2: a DesktopView(16r10010, 'Desktop')
00000041 [6076] M:View>>position: A1: 687@442

D7.1.20 : Dialog should (and does) open on left (display 1) montitor. 00000000 [16840] M:ShellView>>createAt:extent: A1: 1919@10 A2: 305@170
00000001 [16840] M:View>>createAt:extent: A1: 1919@10 A2: 305@170
00000002 [16840] M:DialogView>>basicCreateAt:extent: A1: 1919@10 A2: 305@170 ... 00000017 [16840] M:View>>centerExtent:within: A1: 305@170 A2: a DesktopView(16r10010, 'Desktop') 00000018 [16840] M:View>>centerExtent:within: A1: 305@170 A2: a DesktopView(16r10010, 'Desktop') 00000019 [16840] M:View>>position: A1: 687@440

D7.1.20 : Dialog should open on right (display 2) montitor, but opens on display 1 instead. 00000020 [16840] M:ShellView>>createAt:extent: A1: 1919@10 A2: 305@170 00000021 [16840] M:View>>createAt:extent: A1: 1919@10 A2: 305@170
00000022 [16840] M:DialogView>>basicCreateAt:extent: A1: 1919@10 A2: 305@170
... 00000037 [16840] M:View>>centerExtent:within: A1: 305@170 A2: a DesktopView(16r10010, 'Desktop') 00000038 [16840] M:View>>centerExtent:within: A1: 305@170 A2: a DesktopView(16r10010, 'Desktop') 00000039 [16840] M:View>>position: A1: 687@440

The final messages are very similar for both monitors and both images. There is only one DesktopView in both D6 and D7, and the final position set is the same on both monitors. Is there some multi-monitor position mapping I have missed or a low-level display affinity setting? Any pointers for further debugging?

blairmcg commented 3 years ago

I don't currently have a two monitor setup (which makes my ability to reproduce this rather limited unfortunately), but if it goes back as far as you say then I am surprised I didn't notice it in the past when I did have two monitors.

From a brief inspection of the code, there is no obvious difference in how DialogView>>showModalTo: attempts to position the dialog. Your traces confirm that. This does make me wonder if it is to do with Windows compatibility settings in the exe manifest, or more importantly lack of them in D6. The code as is predates any consideration of multiple monitors, but Windows may be helping out with D6. The Microsoft docs state that a dialog box:

Appears on the monitor of the window that owns it.If it is defined with DS_CENTERMOUSE style [not true of the Dolphin template], it appears on the monitor with the mouse. If it has no owner and the active window and the dialog box are in the same application, the dialog box appears on the monitor of the currently active window. If the dialog box has no owner and the active window is not in the same application as the dialog box, the dialog box appears on the primary monitor.

Based on this description we would expect the dialog to appear on the same monitor as the active Dolphin window by default, but obviously the centring code is repositioning it. I suspect there may be some difference of behaviour in an application which does not specify that it is compatible with more recent versions of Windows like D6, vs one that does, like D7. I haven't been able to find any suggestion of this in the documentation, however, so I might well be wrong, but can't investigate any further at present with my current setup. You might like to try just commenting out the code that centres the dialog within the desktop area to see how this affects things in both cases. I don't think it works as originally intended anyway, even in D6. It looks like it was intended to centre the dialog within the owner, but ends up just centring in the desktop, since the creationParent is always the desktop, in which case it should actually be written more like:

        ...
    self isInitiallyCentered
        ifTrue: 
            [self position: (owningView mapPoint: (self centerExtent: self extent within: owningView)
                        to: creationParent)].
       ...
cDemersMitSci commented 3 years ago

I've had two monitors for years and didn't really notice (or at least didn't think much about) this issue until an associate was trying to use a laptop docked to a monitor. Dialogs kept opening on his laptop rather than his desktop monitor. Obviously he found this frustrating.

I tried removing the final position setting (an earlier position was required, but seemingly disregarded in DialogView>>basicCreateAt:extent: ). That didn't fix the problem.

Then I tried your code change above, and that fixed the problem. It is also probably a more desirable position for Dialogs (centered over owningView) anyway. I am going to kick the image around over here for a bit. If I don't see any problems I'll submit the code changes through Git.

There may be a further concern about where Shells should open. Presently they open on the primary monitor. I don't think I am too concerned about this yet. That could be a more complicated change.

As part of my debugging effort I did add the following Windows APIs: UserLibrary -> #getMonitorInfo:lpmi: UserLibrary -> #monitorFromWindow:dword:

They let me get the coordinates of the monitor containing a window. That might help with default positioning of Shells.

Thank you so much for your assistance with this issue and for keeping the Dolphin torch burning.

blairmcg commented 3 years ago

That's good. I did think that mapping from the co-ordinate space of the owner to the creation parent (which is always the desktop) might fix it.

The more I look at it the more I think this is closer to the original intent anyway, e.g. see the comment on ShellView>>#isInitiallyCentered). Based on such the comments, and some vague recollections, I suspect the code might have been written originally such that the creationParent would have been the owner, but this wouldn't have worked well as there is an expectation in the framework that there is a true parent-child relationship, which is not the case for the ownership relationship between top-level windows. The code goes back to the very early days (early 90's) so recollections are very hazy.

As I think it is the right fix, I'll send a PR for it anyway, but first I'll have to come up with a regression test.

cDemersMitSci commented 3 years ago

I just wanted to offer a heads up about something we encountered with the original fix. It worked great in a deployed EXE, however there was a case where we can have a modal Dialog open before any other Dolphin Shells. When that happened the Dialog was centered on 0@0 because View active returned a NULL View.

It looks like you are doing some extra checking to make sure the view is not off the screen, so your code may already catch this.

Here is how I manually tested this in an image: [(Delay forSeconds: 5) wait. av := View active. Dialog showModal. ] fork. "Click on a blank area of the Desktop to remove focus from the active view." I solved this by checking if the owningView isOpen if false I'm using View<<centerOnActiveMonitorExtent: which is a method I added that I'm now also using to open Shells on the active monitor. I'll include my code below, but I see that you have already refactored and expanded on the fix so you may have the covered.

`!View methodsFor!

centerOnActiveMonitorExtent: aPoint

| rect |

rect := Rectangle center: View desktop activeMonitorRectangle center extent: aPoint.
^rect origin

! ! !View categoriesFor: #centerOnActiveMonitorExtent:!geometry!private! !

!DesktopView methodsFor!

activeMonitorRectangle

"cdemers 6/14/2021 Return the active monitor rectangle (the monitor with the mouse cursor)."
| point hMonitor monInfo |

point := (POINTL fromPoint: Cursor position).
hMonitor := UserLibrary default monitorFromPoint: point dwFlags: 0.
monInfo := MONITORINFO new.
monInfo cbSize: monInfo size.
UserLibrary default getMonitorInfo: hMonitor lpmi: monInfo.
^monInfo rcMonitor asRectangle! !

!DesktopView categoriesFor: #activeMonitorRectangle!public! !

`

I've also made this change to View<<defaultPositionWithin:forExtent: that now centers Shells on whichever monitor the mouse cursor is on: `defaultPositionWithin: aParentView forExtent: aPoint "Private - Answer the default position of the receiver within aParentView given its intended extent aPoint. This is used only on creation."

| testWindow defaultPosition |
self assert: [aParentView notNil].
"cdemers 6/14/2021 Now try to center shells on the active monitor."
self isInitiallyCentered ifTrue: [^(aParentView  == self class desktop) 
            ifTrue: [self centerOnActiveMonitorExtent: aPoint] ifFalse: [self centerExtent: aPoint within: aParentView]].

`...

Since you are working in this area it might be good to have Shells open in the active monitor as well. With these changes both Dialogs and Shells work well on multiple monitor systems. I'm patching an older version of Dolphin so I'm trying to make as few changes as I can.

blairmcg commented 3 years ago

I just wanted to offer a heads up about something we encountered with the original fix. It worked great in a deployed EXE, however there was a case where we can have a modal Dialog open before any other Dolphin Shells. When that happened the Dialog was centered on 0@0 because View active returned a NULL View.

That's a good catch Chris. I think that is a bug in View active - it should return nil, and then the dialog activation code should respond accordingly. I did wonder briefly whether it should return the desktop view when the GetActiveWindow API returns a null handle, but that isn't really correct. The desktop itself is never really active - when you click on the desktop you actually activate the Windows 'Program Manager' (i.e. the shell application). GetActiveWindow should only return windows from the calling process, so nil would be more correct, although a potentially breaking change.

It is possible that application code relies on the existing behaviour of View class>>active return an invalid null View as that will work in some circumstances (such as here in the DialogView code), but accessing the active view directly is fairly unusual, so I am considering changing it to return nil. I will certainly fix it in the master branch.

It looks like you are doing some extra checking to make sure the view is not off the screen, so your code may already catch this.

It does prevent the dialog appearing partly off-screen, yes, but the positioning is far from ideal (top left of primary monitor).

A way to identify the active monitor is a good idea. I think perhaps it needs to be a little more nuanced than just the display with the mouse cursor, since that might not be the display the user is actually working on at the time. For example let's say I set focus to some window, but push the mouse (perhaps accidentally) into another display. The display that should have my attention is the one with the foreground window and focus, as that is where my typing will go. If I use a keyboard shortcut to open a further window, it should go on that monitor by preference. Assuming the DisplayMonitor abstraction in my PR it would be something like:

DisplayMonitor class>>active
    "Answer an instance of the receiver representing the monitor that is most likely to be the one the user might consider active. There is no completely reliable way to determine this, so we use the heuristic that the monitor of the foreground window (whether that is a Dolphin window or not) is likely to be the one with the users' interest."

    ^View foregroundHandle
        ifNotNil: [:hWnd | self displayingView: hWnd]
        ifNil: 
            ["It is unlikely that GetForegroundWindow will return null, but according to the docs this can be the case if activation is in transition, so in that case fall back on the display with the mouse cursor."
            self containingPoint: Cursor position]

This won't always be right, as sometimes the display with the cursor will be better, but I think it is probably more likely to be right in more cases.

Since you are working in this area it might be good to have Shells open in the active monitor as well. With these changes both Dialogs and Shells work well on multiple monitor systems.

I agree, but I will look at the non-dialog case separately.

blairmcg commented 3 years ago

I found a small issue with the fix, which can be seen by placing a View Composer so that it is split between screens, but mainly on the screen to the left, but with the property inspector on the monitor to the right. When one of the in-place editor dialogs is opened from the property inspector, the dialog comes up on the left monitor. This is because it gets repositioned on to the monitor with which the window is associated, which is that monitor on which the window is mostly displayed.

blairmcg commented 3 years ago

Regression fixed and tested by 06cbc94