Bug 310155 - Incorrect Yakuake height handling on dualhead
Summary: Incorrect Yakuake height handling on dualhead
Status: RESOLVED FIXED
Alias: None
Product: yakuake
Classification: Applications
Component: general (show other bugs)
Version: 2.9.9
Platform: Fedora RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Eike Hein
URL:
Keywords:
: 308866 310324 318704 318862 322867 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-11-15 15:53 UTC by Martin Kyral
Modified: 2016-10-11 08:03 UTC (History)
16 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Screenshot of the behavior (487.15 KB, image/png)
2012-11-15 15:56 UTC, Martin Kyral
Details
Yakuake on KDE 4.9.95 (102.97 KB, image/png)
2013-01-03 16:09 UTC, Martin Kyral
Details
patch tha fixes this problem in two different dual head setups. (1.48 KB, patch)
2014-11-22 01:57 UTC, Vangelis
Details
"Improved" patch (1.46 KB, patch)
2014-11-22 21:58 UTC, Vangelis
Details
Using KWindowInfo::isOnCurrentDesktop() (1.46 KB, patch)
2014-11-22 22:39 UTC, Vangelis
Details
patch generated with git show > Yakuake.patch (1.67 KB, patch)
2014-11-22 23:00 UTC, Vangelis
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Kyral 2012-11-15 15:53:11 UTC
With two displays (I have 1366x768 as primary and 1920x1080 as secondary) yakuake doesn't handle height correctly (see Steps to Reproduce)

Reproducible: Always

Steps to Reproduce:
1. Run/Move yakuake on the secondary (larger) display
2. Press F12 (show yakuake).
3. Press F12 twice (to hide and show yakuake again).
4. Change the window size using the shotcut either way.
5. Press F12 twice (to hide and show yakuake again).
Actual Results:  
2. Yakuake shows up with the height set according the primary display height (ie. smaller than expected).
3. Yakuake shows up with the height set according the complete (not counting the bottom panel, if present - for 100%, bottom of the yakuake window incl. the controls is hidden under the panel) secondary display height.
4. Yakuake shrinks its height according the primary display height (ie. smaller than expected).
5. Yakuake shows up with the height set according the complete (not counting the bottom panel, if present - for 100%, bottom of the yakuake window incl. the controls is hidden under the panel) secondary display height.


Expected Results:  
2.,3.,4.,5. - Yakuake is shown with height set according to correct secondary display's height with the bottom panel height counted into account.

Width is handled correctly.
Comment 1 Martin Kyral 2012-11-15 15:56:49 UTC
Created attachment 75284 [details]
Screenshot of the behavior

yakuake window is too tall, the tabs and controls are hidden underneath the panel.
Comment 2 Eike Hein 2012-11-15 16:04:04 UTC
Haven't looked into it yet, but FWIW, thanks for a high-quality bug report.
Comment 3 Eike Hein 2012-11-19 19:31:24 UTC
*** Bug 310324 has been marked as a duplicate of this bug. ***
Comment 4 Martin Kyral 2013-01-03 16:07:08 UTC
I've just updated to KDE 4.9.95 (along with Fedora 17 -> Fedora 18 update) and the behavior changed.
Now, Yakuake height is almost always set according to the primary / smaller display height with the respect of the plasma panel height. In my case it means that on the primary display (notebook built-in 1366x768) the behavior is always correct. However on the secondary, the yakuake window is too small (see screenshot, yakuake height is set to 100%), with the only exception - if the yakuake window width is changed while yakuake window is visible (on the secondary / larger display), it changes its height to the correct one (ie. full display height with the panel taken into account, nothing hidden beneath). After hiding and showing (press F12 twice) yakuake shows again too small.
Comment 5 Martin Kyral 2013-01-03 16:09:08 UTC
Created attachment 76165 [details]
Yakuake on KDE 4.9.95

shown on secondary display 1920x1080 (with primary 1366x768)
Comment 6 Martin Kyral 2013-01-08 14:10:44 UTC
The problem is somewhere in MainWindow::getDesktopGeometry(), in the 'if (KApplication::desktop()->numScreens() > 1)' branch. Unfortunatelly ATM, I don't understand the code enough to point better or provide patch. screenGeometry before the branch for 'numScreens() > 1' is correct (not counting the bottom panel height) -> ie. fullscreen works.
Comment 7 Martin Kyral 2013-01-08 16:52:06 UTC
In my case in KWindowSystem::workArea(offScreenWindows, currentDesktop).intersect(screenGeometry) the data is:

workArea(..): height: 736, width: 3286 
screenGeometry: height: 1080, width: 1920

The intersection then has the following dimensions height: 736, width: 1920
Comment 8 Viranch Mehta 2013-03-18 14:49:49 UTC
I can confirm this bug on ArchLinux latest packages too.
Comment 9 Eric Donkersloot 2013-03-20 10:24:31 UTC
I can also confirm this bug, using the following packages:

yakuake 2.9.9-1
kdebase-workspace 4.10.1-1
Comment 10 Nick 2013-04-26 15:20:10 UTC
I can also confirm with Kubuntu 13.04

kdebase-workspace 4.10.2
yakuake 2.9.9
Comment 11 Nick 2013-04-29 08:31:20 UTC
I should have said downgrading yakuake to 2.9.8 fixes the problem
Comment 12 Eike Hein 2014-10-19 09:58:04 UTC
*** Bug 318704 has been marked as a duplicate of this bug. ***
Comment 13 Eike Hein 2014-10-19 09:58:27 UTC
*** Bug 318862 has been marked as a duplicate of this bug. ***
Comment 14 Eike Hein 2014-10-19 09:59:02 UTC
*** Bug 322867 has been marked as a duplicate of this bug. ***
Comment 15 Eike Hein 2014-10-19 09:59:32 UTC
*** Bug 308866 has been marked as a duplicate of this bug. ***
Comment 16 Vangelis 2014-11-22 01:57:29 UTC
Created attachment 89675 [details]
patch tha fixes this problem in two different dual head setups.

After some testing, I see that the line:

int currentDesktop = KWindowSystem::windowInfo(winId(), NET::WMDesktop).desktop();

returns -1 the first time Yakuake is toggled (or when the settings are updated) and for all of the subsequent toggles, 0 is returned.

Moreover, if you split the above line in a few lines like this:
KWindowInfo windowInfo = KWindowSystem::windowInfo(winId(), NET::WMDesktop);
int currentDesktop = 1000;
if(windowInfo.valid())
     currentDesktop = windowInfo..desktop();
else
     qDebug() << "NOT VALID :(";

You will see that the windowInfo.valid() always returns FALSE!
So it looks like this line is useless.

After adding some qDebug() messages in the while loop, I saw that the function KWindowInfo.desktop(), always returned -1 for window IDs running in the currentDesktop, and the number of the desktop for the rest of the desktops.
So it looks like the currentDesktop variable is not needed at all and it can "safely" be replaced with -1.

The attached patch fixes the problem for me in two different laptops running in two different dual head configurations.
Comment 17 Eike Hein 2014-11-22 12:52:36 UTC
Thanks for looking into it! Unfortunately that's not really an acceptable fix, we'll rather have to address the timing issue that causes the window info to be invalid in the first place. We could probably do that by just using KWindowSystem::currentDesktop(), taking the state of Yakuake's window mapping out of the equation - I probably wrote it this way in case Yakuake would ever not be on the actually-current desktop by the time this code runs, but off-hand I can't actually think of a scenario where this would be either true, or matter.

As for KWindowInfo.desktop() always returning -1, we should probably port it to KWindowInfo::isOnDesktop(currentDesktop), then we can avoid hardcoding any sort of number in our code.

Would you like to check that those two things work for you?
Comment 18 Vangelis 2014-11-22 21:58:36 UTC
Created attachment 89687 [details]
"Improved" patch

Hi Eike,

Your concerns sound reasonable :)

How about this one?

We get the currentDesktop from KWindowSystem::currentDesktop() and we deal with the struts from windows located in the current desktop if (windowInfo.valid() && windowInfo.isOnDesktop(currentDesktop))
Comment 19 Vangelis 2014-11-22 22:38:00 UTC
I just saw that there is a function "KWindowInfo::isOnCurrentDesktop()"

http://api.kde.org/4.14-api/kdelibs-apidocs/kdeui/html/classKWindowInfo.html#ac5d51bdc5ebbe8c8eae00e422c134127

If we use this one, then we do not need the currentDesktop variable at all.

KWindowSystem::workArea() by default (without a passed parameter) returns the workArea of the current desktop.







--- yakuake-2.9.9-a/app/mainwindow.cpp  2014-11-22 22:49:39.297582382 +0100
+++ yakuake-2.9.9-b/app/mainwindow.cpp  2014-11-22 23:33:09.566934201 +0100
@@ -1265,8 +1265,6 @@ QRect MainWindow::getDesktopGeometry()
     if (action->isChecked())
         return screenGeometry;
 
-    int currentDesktop = KWindowSystem::windowInfo(winId(), NET::WMDesktop).desktop();
-
     if (KApplication::desktop()->numScreens() > 1)
     {
         const QList<WId> allWindows = KWindowSystem::windows();
@@ -1282,7 +1280,9 @@ QRect MainWindow::getDesktopGeometry()
             {
                 KWindowInfo windowInfo = KWindowSystem::windowInfo(windowId, NET::WMDesktop, NET::WM2ExtendedStrut);
 
-                if (windowInfo.valid() && windowInfo.desktop() == currentDesktop)
+                // If windowInfo is valid and the window is located at the same (current)
+                // desktop with the yakuake window...
+                if (windowInfo.valid() && windowInfo.isOnCurrentDesktop())
                 {
                     NETExtendedStrut strut = windowInfo.extendedStrut();
 
@@ -1312,10 +1312,10 @@ QRect MainWindow::getDesktopGeometry()
             }
         }
 
-        return KWindowSystem::workArea(offScreenWindows, currentDesktop).intersect(screenGeometry);
+        return KWindowSystem::workArea(offScreenWindows).intersect(screenGeometry);
     }
 
-    return KWindowSystem::workArea(currentDesktop);
+    return KWindowSystem::workArea();
 }
 
 void MainWindow::whatsThis()
Comment 20 Vangelis 2014-11-22 22:39:41 UTC
Created attachment 89688 [details]
Using KWindowInfo::isOnCurrentDesktop()
Comment 21 Eike Hein 2014-11-22 22:43:31 UTC
Looks good - do you have a KDE dev account to commit?
Comment 22 Vangelis 2014-11-22 22:44:33 UTC
(In reply to Eike Hein from comment #21)
> Looks good - do you have a KDE dev account to commit?

Nope, but you can do it! No problem :)
Comment 23 Eike Hein 2014-11-22 22:45:30 UTC
Do you want to provide the patch in git format (make a local commit, git show > file) so your author metadata is preserved?
Comment 24 Vangelis 2014-11-22 23:00:57 UTC
Created attachment 89689 [details]
patch generated with git show > Yakuake.patch

I didn't know that I could supply a patch like this with git.
Hope it fine.

Thanks :)
Comment 25 Eike Hein 2014-11-23 00:23:42 UTC
Git commit 5e31ccfdbc3a46e6bffd79fc6bcafc3a2bde0ed3 by Eike Hein, on behalf of Vangelis Tasoulas.
Committed on 22/11/2014 at 23:16.
Pushed by hein into branch 'master'.

Fixes incorrect Yakuake height handling on dualhead setups.

M  +5    -5    app/mainwindow.cpp

http://commits.kde.org/yakuake/5e31ccfdbc3a46e6bffd79fc6bcafc3a2bde0ed3
Comment 26 Lisandro Damián Nicanor Pérez Meyer 2014-11-24 18:38:12 UTC
I backported it to 2.9.9 and seems to work :-D

Thanks a lot!
Comment 27 J.D. 2015-09-01 07:42:05 UTC
I'm running 2.9.9 on OpenSUSE (KDE 4.14.9) and still the screen hight changes correctly when first set but the next time it is toggled it resizes according to the hight of the fist screen. This is a laptop dual-screen setup with nvidia display driver. Are you sure the patch is applied to 2.9.9?
Comment 28 Heiko Tietze 2015-11-15 10:52:47 UTC
It's still an issue. Same config as described above, and yakuake pops up at the wrong position.

>plasmashell -v
plasmashell 5.4.3

>qtdiag-qt5
Qt 5.5.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.2.0) on "xcb" 
OS: Arch Linux [linux version 4.2.5-1-ARCH]
Comment 29 Zach 2015-11-23 09:56:21 UTC
Can confirm its still a problem on plasma 5.  Using Kubuntu 15.10.  When the height is set, it adjusts properly, but the next time it opens, the height change does not take affect.
Comment 30 Alexey 2016-02-03 09:09:05 UTC
Arch linux
plasmashell 5.5.4
Qt 5.5.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.3.0) on "xcb"
Yakuake: 2.9.9
Problem still exist.
Comment 31 Marek Bettman 2016-10-11 08:03:11 UTC
Still there on Kubuntu 16.04.1 64bit

$ qtdiag
Qt 5.5.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.3.1 20160413) on "xcb" 
OS: Ubuntu 16.04.1 LTS [linux version 4.4.0-38-generic]

yakuake 2.9.9-3

plasma-desktop 4:5.5.5-0ubuntu1
plasma-workspace 4:5.5.5.2-0ubuntu1
$ plasmashell -v
plasmashell 5.5.5

Nevermind, which screen is set as primary in System Settings, it always falls back to the smaller one.