Bug #725

gdk_screen_get_primary_monitor only works with GTK>=2.19.2

Added by Jan DolinĂ¡r over 6 years ago. Updated over 6 years ago.

Status:ApprovedStart date:03/16/2014
Priority:NormalDue date:
Assignee:Miroslav Fidler% Done:

0%

Category:CtrlCoreSpent time:-
Target version:-

Description

The recently introduced changes in GetPrimaryWorkArea (#640, r6761) use function gdk_screen_get_primary_monitor which only exists since GTK 2.19.2. The compilation fails for older versions of GTK:

[  218s] /home/abuild/rpmbuild/BUILD/upp-x11-src-7041/uppsrc/CtrlCore/GtkWnd.cpp: In static member function 'static Upp::Rect Upp::Ctrl::GetPrimaryWorkArea()':
[  218s] /home/abuild/rpmbuild/BUILD/upp-x11-src-7041/uppsrc/CtrlCore/GtkWnd.cpp:294: error: 'gdk_screen_get_primary_monitor' was not declared in this scope

I found this while trying to compile TheIDE for ScientificLinux, which has gtk2-devel package only in version 2.18.9. There might also be other affected distros...

We could wrap it in #ifdef GTK_VERSION_CHECK(2,19,2) and provide the previous behavior as a fallback, what do you think?

GtkWnd.cpp Magnifier (10.9 KB) Zbigniew Rebacz, 03/17/2014 04:53 PM

X11App.cpp Magnifier - Remove "SyncMousePos" from GetWorkArea method. (13.2 KB) Zbigniew Rebacz, 03/28/2014 11:12 PM


Related issues

Related to Bug #707: GetWorkArea() should track mouse cursor position (GTK ba... Rejected 03/02/2014

History

#1 Updated by Zbigniew Rebacz over 6 years ago

  • File GtkWnd.cpp added
  • Status changed from New to Patch ready

It seems that this extension works since version 2.20.0: https://developer.gnome.org/gdk3/unstable/GdkScreen.html#gdk-screen-get-primary-monitor.

For me the easiest solution should look like this:

Rect Ctrl::GetPrimaryWorkArea()
{
    GuiLock __;
    static Rect r;
    if (r.right == 0) {
        Array<Rect> rc;
        GetWorkArea(rc);
#if GTK_CHECK_VERSION(2, 20, 0)
        int primary = gdk_screen_get_primary_monitor(gdk_screen_get_default());
#else
        int primary = 0;
#endif
        primary >= 0 && primary < rc.GetCount() ? r = rc[primary] : r = GetVirtualScreenArea();
    }
    return r;
}

Please noticed that this patch will affect my uncomited patch: http://www.ultimatepp.org/redmine/issues/707#change-1522. So, I decided to included "latest" version of "GTKWnd.cpp".

#2 Updated by Zbigniew Rebacz over 6 years ago

  • File deleted (GtkWnd.cpp)

#3 Updated by Zbigniew Rebacz over 6 years ago

I think we can also improve a little bit X11 version of following method: "Rect Ctrl::GetPrimaryWorkArea()".

#4 Updated by Miroslav Fidler over 6 years ago

  • Status changed from Patch ready to Ready for CR
  • Assignee changed from Miroslav Fidler to Zbigniew Rebacz

Applied, however:

GetWorkArea docs (and original intent) is that the area that the window belongs to is returned, not mouse position. This is also consistent with Win32 behaviour.

Therefore, I have (and in X11 too) done this change:

[code]
Rect Ctrl::GetWorkArea() const {
GuiLock __;
static Array<Rect> rc;
if(rc.IsEmpty())
GetWorkArea(rc);

Point pt = GetScreenRect().TopLeft();
for (int i = 0; i < rc.GetCount(); i++)
if(rc[i].Contains(pt))
return rc[i];
return GetPrimaryWorkArea();
}
[/code]

I know that this is not bulletproof either (I guess window can be on more screens), but at least it follow original U++ docs (written at the time of adding this in Win32).

#5 Updated by Zbigniew Rebacz over 6 years ago

  • File X11App.cppMagnifier added
  • Assignee changed from Zbigniew Rebacz to Miroslav Fidler

It seems that TopWindow::CenterRect for X11 needs improvement. It seems that it is not ready for GetScreenRect(). (X11Top.cpp)

#6 Updated by Miroslav Fidler over 6 years ago

  • Status changed from Ready for CR to Approved

#7 Updated by Zbigniew Rebacz over 6 years ago

  • File deleted (X11Top.cpp)

#8 Updated by Zbigniew Rebacz over 6 years ago

  • Status changed from Approved to Ready for CR

In my opinion "GetRect().TopLeft()" is not perfect solution, because it causes several problems like tool tips are show only on one screen or similar error with "DisplayPopup" (It can be seen in ArrayCtrl).

For me the most stable and user friendly solution is to check absolute mouse possition on both platforms X11 & GTK. Moreover it gaves the best results for tool tips (They are always fit to current screen even if the window is on the edge between two screen). Maybe we should implemented similar solution on M$ Windows.

For the end I would like to write that I am almost sure that GetWorkArea() should return dynamic results. Mouse possition seems to be most resonable.

// **
If you decided to use my solution, please patch also X11Top.cpp (line 254 GetVirtualScreenArea() -> GetWorkArea()).

#9 Updated by Miroslav Fidler over 6 years ago

Well, but perhaps it is then rather error of DisplayPopup or tips. I mean, perhaps they should rather find proper work area based on mouse position, but meanwhile current method should stay as it is...

Mirek

#10 Updated by Zbigniew Rebacz over 6 years ago

For me, we can now close this bug. It seems to me that now I understand the design. I understand that we need also patch "Display Popup" & "ToolTip". (I will create new bug report for this issues)

I have got one question. What about Windows version of method "GetWorkArea"? For me the implementation is strange and probably incompatible with the upp documentation.

Rect Ctrl::GetWorkArea() const
{
// return MonitorRectForHWND(GetHWND());
// mst:2008-12-08, hack for better multimonitor support.
    GuiLock __;
    const Ctrl *topctl = GetTopCtrl();
    HWND hwnd = topctl->GetHWND();
    if(!hwnd && !((topctl = topctl->GetOwnerCtrl()) && (hwnd = topctl->GetHWND())))
        hwnd = ::GetFocus();
    return MonitorRectForHWND(hwnd);
}

For me the implementation of "GetWorkArea" should works exactly the same on all platforms.

Last but not least is thing that you can remove "SyncMousePos()" from "GetWorkArea" implementation on X11 (line 538). It is not needed anymore.

#11 Updated by Zbigniew Rebacz over 6 years ago

  • File deleted (X11App.cpp)

#12 Updated by Zbigniew Rebacz over 6 years ago

#13 Updated by Miroslav Fidler over 6 years ago

  • Status changed from Ready for CR to Approved

Also available in: Atom PDF