Re: Review Request 127424: KCompletionBox popup gets full window decoration on Windows

[Thomas Lübking]

> On März 21, 2016, 8:42 nachm., Dominik Haumann wrote:
> > ping
> 
> Marco Martin wrote:
> +1, to me the latest version could be fine, would like a last word from 
> windowmanager people tough

As mentioned, I ceased to care.
I'm providing input while phasing out (and promise it to be on best knowledge) 
but won't take decisions which do no longer affect me in their consequences.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127424/#review93837
---


On März 20, 2016, 12:06 nachm., Dominik Haumann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127424/
> ---
> 
> (Updated März 20, 2016, 12:06 nachm.)
> 
> 
> Review request for KDE Frameworks, kdewin, kwin, and Marco Martin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> In https://git.reviewboard.kde.org/r/127191/ the KCompletionBox WindowFlags 
> were change from Qt::ToolTip to Qt::Window.
> 
> As consequence, the completion popup of the Kate command line gets a full 
> window decoration, which is obviously wrong, see attached screenshot.
> 
> Changing the type to Qt::Popup shows the proper popup, but key presses are 
> not forwarded, so typing is not possible, and additionally, on losing focus 
> the popup keeps staying open.
> Therefore, this patch reverts the type back to Qt::ToolTip.
> 
> Better fixes are of course welcome ;) Any ideas?
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 005aff8 
> 
> Diff: https://git.reviewboard.kde.org/r/127424/diff/
> 
> 
> Testing
> ---
> 
> Works on Windows as expected.
> 
> 
> File Attachments
> 
> 
> Completion Popup
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/7be64cad-6d95-46b8-9caa-41b41a135ca1__kate2015.png
> 
> 
> Thanks,
> 
> Dominik Haumann
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127424: KCompletionBox popup gets full window decoration on Windows

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127424/#review93780
---



except that it's effectively an editable combobox?

_NET_WM_WINDOW_TYPE_COMBO should be used on the windows that are popped up by 
combo boxes. An example is a window that appears below a text field with a list 
of suggested completions. This property is typically used on override-redirect 
windows.

- Thomas Lübking


On März 20, 2016, 12:06 nachm., Dominik Haumann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127424/
> ---
> 
> (Updated März 20, 2016, 12:06 nachm.)
> 
> 
> Review request for KDE Frameworks, kdewin, kwin, and Marco Martin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> In https://git.reviewboard.kde.org/r/127191/ the KCompletionBox WindowFlags 
> were change from Qt::ToolTip to Qt::Window.
> 
> As consequence, the completion popup of the Kate command line gets a full 
> window decoration, which is obviously wrong, see attached screenshot.
> 
> Changing the type to Qt::Popup shows the proper popup, but key presses are 
> not forwarded, so typing is not possible, and additionally, on losing focus 
> the popup keeps staying open.
> Therefore, this patch reverts the type back to Qt::ToolTip.
> 
> Better fixes are of course welcome ;) Any ideas?
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 005aff8 
> 
> Diff: https://git.reviewboard.kde.org/r/127424/diff/
> 
> 
> Testing
> ---
> 
> Works on Windows as expected.
> 
> 
> File Attachments
> 
> 
> Completion Popup
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/7be64cad-6d95-46b8-9caa-41b41a135ca1__kate2015.png
> 
> 
> Thanks,
> 
> Dominik Haumann
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127424: KCompletionBox popup gets full window decoration on Windows

[Thomas Lübking]

> On März 20, 2016, 4:22 vorm., Anthony Fieroni wrote:
> > src/kcompletionbox.cpp, line 282
> > 
> >
> > After show, call activateWindow(), even you can call raise()

The window shall *not* be activated.
One could probably call activateWindow() on the parenting/filtered window and 
raise the completionbox afterwards (because it still needs to be above the main 
window), but I frankly ceased to care.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127424/#review93749
---


On März 19, 2016, 8:56 nachm., Dominik Haumann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127424/
> ---
> 
> (Updated März 19, 2016, 8:56 nachm.)
> 
> 
> Review request for KDE Frameworks, kdewin, kwin, and Marco Martin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> In https://git.reviewboard.kde.org/r/127191/ the KCompletionBox WindowFlags 
> were change from Qt::ToolTip to Qt::Window.
> 
> As consequence, the completion popup of the Kate command line gets a full 
> window decoration, which is obviously wrong, see attached screenshot.
> 
> Changing the type to Qt::Popup shows the proper popup, but key presses are 
> not forwarded, so typing is not possible, and additionally, on losing focus 
> the popup keeps staying open.
> Therefore, this patch reverts the type back to Qt::ToolTip.
> 
> Better fixes are of course welcome ;) Any ideas?
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 005aff8 
> 
> Diff: https://git.reviewboard.kde.org/r/127424/diff/
> 
> 
> Testing
> ---
> 
> Works on Windows as expected.
> 
> 
> File Attachments
> 
> 
> Completion Popup
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/7be64cad-6d95-46b8-9caa-41b41a135ca1__kate2015.png
> 
> 
> Thanks,
> 
> Dominik Haumann
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127424: KCompletionBox popup gets full window decoration on Windows

[Thomas Lübking]

> On März 19, 2016, 1:22 nachm., Thomas Lübking wrote:
> > "Loose focus" is the complete wrong concept anyway.
> > It makes kickoff and various other plasmashell elements barely usable w/ 
> > any but the click-to-focus policy (implicit hide)
> > 
> > Try adding Qt::FramelessWindowHint (Qt::X11BypassWindowManagerHint should 
> > be Qt::BypassWindowManagerHint anyway)
> > 
> > PS: it's rather worrysome that Qt::Popup fails since it *is* set for the 
> > QComboBox dropdown. May point an implementation weakeness/bug in this class.
> 
> Dominik Haumann wrote:
> Unfortunatey, also does not work... The popup stays, does not close, and 
> typing does not insert text into the KLineEdit anymore (we don't see any 
> blinking cursor).
> 
> Thomas Lübking wrote:
> i did not mean "alongside Qt::Popup" - if you need to hint this as 
> tooltip for somewhat useful behavior, either the class or QWidget/5 is 
> seriously fucked up.
> 
> Dominik Haumann wrote:
> ...I'm not sure what you are suggesting: Fact is, right now, we don't 
> have another fix than reverting to the original code.
> It would also work to add a #ifdef, but given that temporary workarounds 
> tend to become pernanent ones, this is probably the worst solution.
> 
> So what is the proper fix / approach we can take here?
> 
> Anthony Fieroni wrote:
> http://doc.qt.io/qt-5/qt.html#WindowType-enum
> q->setWindowFlags(Qt::ToolTip | Qt::BypassWindowManagerHint | 
> Qt::FramelessWindowHint);
> or 
> q->activateWindow();
> *(i.e., no keyboard input unless you call QWidget::activateWindow() 
> manually).*
> 
> Anthony Fieroni wrote:
> I mean Window :)
> q->setWindowFlags(Qt::Window | Qt::BypassWindowManagerHint | 
> Qt::FramelessWindowHint);
> 
> Dominik Haumann wrote:
> Just tested
> 
> q->setWindowFlags(Qt::Window | Qt::BypassWindowManagerHint | 
> Qt::FramelessWindowHint);
> 
> Does not work either: We get no window decoration, which is good, but 
> KLineEdit looses focus (no blinking cursor, no new typed text), and it is 
> hard to make the popup disappear again.
> 
> Maybe this is a bug in the implementation of KCompletionBox and/or 
> KLineEditor. Fact is, though, that it at least worked for applications before.
> 
> I could not find a call of activateWindow()...
> 
> Kai Uwe Broulik wrote:
> Does Qt::WindowDoesNotAcceptFocus help?
> 
> Dominik Haumann wrote:
> Unfortunately, no, no matter what combinations I try ;) (Qt::Window, 
> Qt::Popup, and the other flags arbitrarily combined)

Qt::WindowDoesNotAcceptFocus only handles what happens if the window receives 
an ::ActivateWindow event, not how the window should be created.

You can try setting ("_q_showWithoutActivating", false) like 
"_q_xcb_wm_window_type", but actually™ when this is tested on the windows 
platform plugin, so is Qt::Popup, Qt::ToolTip and Qt::Tool.
Qt::ToolTip is btw. Qt::Popup|Qt::Sheet and ::Sheet doesn't exist anywhere but 
on OSX.

As ultimate resolution branch #ifdef Q_OS_WIN to tag a combobox drawer a 
tooltip. And better start looking for another toolkit.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127424/#review93716
---


On März 19, 2016, 8:56 nachm., Dominik Haumann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127424/
> ---
> 
> (Updated März 19, 2016, 8:56 nachm.)
> 
> 
> Review request for KDE Frameworks, kdewin, kwin, and Marco Martin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> In https://git.reviewboard.kde.org/r/127191/ the KCompletionBox WindowFlags 
> were change from Qt::ToolTip to Qt::Window.
> 
> As consequence, the completion popup of the Kate command line gets a full 
> window decoration, which is obviously wrong, see attached screenshot.
> 
> Changing the type to Qt::Popup shows the proper popup, but key presses are 
> not forwarded, so typing is not possible, and additionally, on losing focus 
> the popup keeps staying open.
> Therefore, this patch reverts the type back to Qt::ToolTip.
> 
> Better fixes are of course welcome ;) Any ideas?
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 005aff8 
> 
> Diff: https://git.reviewboard.kde.org/r/127424/diff/
> 
> 
> Testing
> ---
> 
&g

Re: Review Request 127424: KCompletionBox popup gets full window decoration on Windows

[Thomas Lübking]

> On März 19, 2016, 1:22 nachm., Thomas Lübking wrote:
> > "Loose focus" is the complete wrong concept anyway.
> > It makes kickoff and various other plasmashell elements barely usable w/ 
> > any but the click-to-focus policy (implicit hide)
> > 
> > Try adding Qt::FramelessWindowHint (Qt::X11BypassWindowManagerHint should 
> > be Qt::BypassWindowManagerHint anyway)
> > 
> > PS: it's rather worrysome that Qt::Popup fails since it *is* set for the 
> > QComboBox dropdown. May point an implementation weakeness/bug in this class.
> 
> Dominik Haumann wrote:
> Unfortunatey, also does not work... The popup stays, does not close, and 
> typing does not insert text into the KLineEdit anymore (we don't see any 
> blinking cursor).

i did not mean "alongside Qt::Popup" - if you need to hint this as tooltip for 
somewhat useful behavior, either the class or QWidget/5 is seriously fucked up.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127424/#review93716
---


On März 19, 2016, Mittag, Dominik Haumann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127424/
> ---
> 
> (Updated März 19, 2016, Mittag)
> 
> 
> Review request for KDE Frameworks, kwin and Marco Martin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> In https://git.reviewboard.kde.org/r/127191/ the KCompletionBox WindowFlags 
> were change from Qt::ToolTip to Qt::Window.
> 
> As consequence, the completion popup of the Kate command line gets a full 
> window decoration, which is obviously wrong, see attached screenshot.
> 
> Changing the type to Qt::Popup shows the proper popup, but key presses are 
> not forwarded, so typing is not possible, and additionally, on losing focus 
> the popup keeps staying open.
> Therefore, this patch reverts the type back to Qt::ToolTip.
> 
> Better fixes are of course welcome ;) Any ideas?
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 005aff8 
> 
> Diff: https://git.reviewboard.kde.org/r/127424/diff/
> 
> 
> Testing
> ---
> 
> Works on Windows as expected.
> 
> 
> File Attachments
> 
> 
> Completion Popup
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/7be64cad-6d95-46b8-9caa-41b41a135ca1__kate2015.png
> 
> 
> Thanks,
> 
> Dominik Haumann
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127424: KCompletionBox popup gets full window decoration on Windows

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127424/#review93716
---



"Loose focus" is the complete wrong concept anyway.
It makes kickoff and various other plasmashell elements barely usable w/ any 
but the click-to-focus policy (implicit hide)

Try adding Qt::FramelessWindowHint (Qt::X11BypassWindowManagerHint should be 
Qt::BypassWindowManagerHint anyway)

PS: it's rather worrysome that Qt::Popup fails since it *is* set for the 
QComboBox dropdown. May point an implementation weakeness/bug in this class.

- Thomas Lübking


On März 19, 2016, Mittag, Dominik Haumann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127424/
> ---
> 
> (Updated März 19, 2016, Mittag)
> 
> 
> Review request for KDE Frameworks, kwin and Marco Martin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> In https://git.reviewboard.kde.org/r/127191/ the KCompletionBox WindowFlags 
> were change from Qt::ToolTip to Qt::Window.
> 
> As consequence, the completion popup of the Kate command line gets a full 
> window decoration, which is obviously wrong, see attached screenshot.
> 
> Changing the type to Qt::Popup shows the proper popup, but key presses are 
> not forwarded, so typing is not possible, and additionally, on losing focus 
> the popup keeps staying open.
> Therefore, this patch reverts the type back to Qt::ToolTip.
> 
> Better fixes are of course welcome ;) Any ideas?
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 005aff8 
> 
> Diff: https://git.reviewboard.kde.org/r/127424/diff/
> 
> 
> Testing
> ---
> 
> Works on Windows as expected.
> 
> 
> File Attachments
> 
> 
> Completion Popup
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/7be64cad-6d95-46b8-9caa-41b41a135ca1__kate2015.png
> 
> 
> Thanks,
> 
> Dominik Haumann
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Thomas Lübking]

> On Feb. 26, 2016, 2:14 p.m., Thomas Lübking wrote:
> > src/kcompletionbox.cpp, line 66
> > <https://git.reviewboard.kde.org/r/127191/diff/1/?file=445519#file445519line66>
> >
> > q->setAttribute(Qt::WA_X11NetWmWindowTypeCombo); // broken??
> 
> Marco Martin wrote:
> ok, i'm blind :p
> 
> Martin Gräßlin wrote:
> cd src/qt5/qtbase/src/plugins/platforms/xcb
> git grep X11NetWmWindowTypeCombo
> 
> -> no result
> 
> I doubt this still works...
> 
> Marco Martin wrote:
> yep, indeed i get type normal now, getting back on the first approach

QComboBox still sets the attribute but it's in a "Q_DEAD_CODE_FROM_QT4_X11" 
preproc branch.
Qt5 is a piece of broken shit - seriously!


QComboBox would then fall to ::setWindowFlags(Qt::Popup) but actually that's 
not worth it at all. The target is QML + Phones, the rest of Qt is apparently 
now unmaintained junk.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127191/#review92809
---


On March 7, 2016, 11:35 a.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated March 7, 2016, 11:35 a.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127191/#review93253
---




src/kcompletionbox.cpp (line 65)
<https://git.reviewboard.kde.org/r/127191/#comment63580>

I doubt this is still required on top, but other than this it's oc. fine 
from my POV, tooltip has been the wrong type for a combo dropdown to begin with.


- Thomas Lübking


On Feb. 26, 2016, 2:18 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated Feb. 26, 2016, 2:18 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

> On March 3, 2016, 10:16 p.m., Thomas Lübking wrote:
> > src/kstatusnotifieritem.cpp, line 934
> > <https://git.reviewboard.kde.org/r/127216/diff/2/?file=446044#file446044line934>
> >
> > append
> > associatedWidget->setAttribute(Qt::WA_Moved);
> > 
> > 
> > 
> > This should tell Qt to demand an explicit position and skip placement 
> > by the WM (yes, this generally works in KWin ;-)
> > 
> > However, this flag should be implicitly set by ::move() unless the 
> > point is invalid, so you might want to "qDebug() << associatedWidgetPos;" 
> > to check whether it's invalid for the failing clients.
> 
> Anthony Fieroni wrote:
> Fun facts :) void KStatusNotifierItemPrivate::minimizeRestore(bool) is 
> called only when you click on tray icon i.e. if i close app with big red X 
> it's not called => i can't store position :)
> 
> Anthony Fieroni wrote:
> More fun facts :) 
> associatedWidget->setGeometry(associatedWidget->frameGeometry()); works in 
> any case ! I will add path if you want, but you will not be happy to set 
> geometry by myself :)
> 
> Anthony Fieroni wrote:
> ^ No, extend geometry with decoration size :)
> 
> Thomas Lübking wrote:
> meehhh...
> 
> That means one will have to filter QEvent::Close of associatedWidget and 
> store the position from there.
> 
> Anthony Fieroni wrote:
> It's strange, about me, associatedWidget has correct frameGeometry even 
> it's hide.
> QObject::connect(KWindowSystem::self(), ::windowRemoved, q, 
> [this](WId winId) {
> if (associatedWidget && associatedWidget->winId() == winId) {
> associatedWidgetPos = 
> associatedWidget->frameGeometry().topLeft();
> }
> });
> Position is correct, but again not work event with Qt::WA_Moved.
> 
> Anthony Fieroni wrote:
> KWindowInfo info(associatedWidget->winId(), NET::WMDesktop | 
> NET::WMGeometry);
> info.geometry().topLeft(); <- decoration is 
> included again
> to filed bug?
> 
> Thomas Lübking wrote:
> You may file a bug, but I frankly doubt your findings.
> (Just more or less ported my kwindowsystem tool and the values are 
> correct ;-)
> 
> => How exactly did you determine this assumption? (Just from the walking 
> position?)
> 
> Anthony Fieroni wrote:
> I can't move widget when new position not match frameGeometry().topLeft 
> by itself.
> associatedWidgetPos = associatedWidget->geometry().topLeft(); -> can move 
> but new position is with decoration offset
> associatedWidgetPos = associatedWidget->pos(); -> can't move, this is 
> wanted position, not match internal wigdet frameGeometry
> I saw qwidget move code, Qt::WA_Moved is setted at start.
> I saw the qwidget close code, because when i click X -> now i CAN move to 
> pos ! Why the hell, i try setAttribute(Qt::WA_QuitOnClose, false);
> What are attribute setted/unsetted on X click?
> 
> Thomas Lübking wrote:
> You'll fail to move because the call is idempotent, ie. the positions is 
> assumed to be the current one and thus the move call is NOOP.
> 
> Qt::WA_QuitOnClose exits the process when the last window closes, leave 
> it alone.

Just tried, Quassel and trojitá (latter uses QSystrayIcon + QPA) actually work 
with the present code.
(Not checked details, but apparently minimizeRestore isn't invoked anyway??)

Trying amarok later (requires complete update for new libav deps) but would 
like to express my greatest disgrace for systrays in general and SNI in 
particular.
The SNI daemon silently quit (not killing kded!) several times! during the 
tests and I spent 5 minutes just to figure why I suddenly wasn't getting any 
tray icon and associated widgets turned 0x0 ... this entire thing is seriously 
far too fragile to be useful. *g*


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93143
---


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.o

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

> On March 3, 2016, 10:16 p.m., Thomas Lübking wrote:
> > src/kstatusnotifieritem.cpp, line 934
> > <https://git.reviewboard.kde.org/r/127216/diff/2/?file=446044#file446044line934>
> >
> > append
> > associatedWidget->setAttribute(Qt::WA_Moved);
> > 
> > 
> > 
> > This should tell Qt to demand an explicit position and skip placement 
> > by the WM (yes, this generally works in KWin ;-)
> > 
> > However, this flag should be implicitly set by ::move() unless the 
> > point is invalid, so you might want to "qDebug() << associatedWidgetPos;" 
> > to check whether it's invalid for the failing clients.
> 
> Anthony Fieroni wrote:
> Fun facts :) void KStatusNotifierItemPrivate::minimizeRestore(bool) is 
> called only when you click on tray icon i.e. if i close app with big red X 
> it's not called => i can't store position :)
> 
> Anthony Fieroni wrote:
> More fun facts :) 
> associatedWidget->setGeometry(associatedWidget->frameGeometry()); works in 
> any case ! I will add path if you want, but you will not be happy to set 
> geometry by myself :)
> 
> Anthony Fieroni wrote:
> ^ No, extend geometry with decoration size :)
> 
> Thomas Lübking wrote:
> meehhh...
> 
> That means one will have to filter QEvent::Close of associatedWidget and 
> store the position from there.
> 
> Anthony Fieroni wrote:
> It's strange, about me, associatedWidget has correct frameGeometry even 
> it's hide.
> QObject::connect(KWindowSystem::self(), ::windowRemoved, q, 
> [this](WId winId) {
> if (associatedWidget && associatedWidget->winId() == winId) {
> associatedWidgetPos = 
> associatedWidget->frameGeometry().topLeft();
> }
> });
> Position is correct, but again not work event with Qt::WA_Moved.
> 
> Anthony Fieroni wrote:
> KWindowInfo info(associatedWidget->winId(), NET::WMDesktop | 
> NET::WMGeometry);
> info.geometry().topLeft(); <- decoration is 
> included again
> to filed bug?
> 
> Thomas Lübking wrote:
> You may file a bug, but I frankly doubt your findings.
> (Just more or less ported my kwindowsystem tool and the values are 
> correct ;-)
> 
> => How exactly did you determine this assumption? (Just from the walking 
> position?)
> 
> Anthony Fieroni wrote:
> I can't move widget when new position not match frameGeometry().topLeft 
> by itself.
> associatedWidgetPos = associatedWidget->geometry().topLeft(); -> can move 
> but new position is with decoration offset
> associatedWidgetPos = associatedWidget->pos(); -> can't move, this is 
> wanted position, not match internal wigdet frameGeometry
> I saw qwidget move code, Qt::WA_Moved is setted at start.
> I saw the qwidget close code, because when i click X -> now i CAN move to 
> pos ! Why the hell, i try setAttribute(Qt::WA_QuitOnClose, false);
> What are attribute setted/unsetted on X click?

You'll fail to move because the call is idempotent, ie. the positions is 
assumed to be the current one and thus the move call is NOOP.

Qt::WA_QuitOnClose exits the process when the last window closes, leave it 
alone.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93143
---


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Store position of widget before hide it
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp cf3e7b5 
>   src/kstatusnotifieritemprivate_p.h 8fdfd4c 
> 
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
> 
> 
> Testing
> ---
> 
> Tested on pixel ration = 1
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

> On March 3, 2016, 10:16 p.m., Thomas Lübking wrote:
> > src/kstatusnotifieritem.cpp, line 934
> > <https://git.reviewboard.kde.org/r/127216/diff/2/?file=446044#file446044line934>
> >
> > append
> > associatedWidget->setAttribute(Qt::WA_Moved);
> > 
> > 
> > 
> > This should tell Qt to demand an explicit position and skip placement 
> > by the WM (yes, this generally works in KWin ;-)
> > 
> > However, this flag should be implicitly set by ::move() unless the 
> > point is invalid, so you might want to "qDebug() << associatedWidgetPos;" 
> > to check whether it's invalid for the failing clients.
> 
> Anthony Fieroni wrote:
> Fun facts :) void KStatusNotifierItemPrivate::minimizeRestore(bool) is 
> called only when you click on tray icon i.e. if i close app with big red X 
> it's not called => i can't store position :)
> 
> Anthony Fieroni wrote:
> More fun facts :) 
> associatedWidget->setGeometry(associatedWidget->frameGeometry()); works in 
> any case ! I will add path if you want, but you will not be happy to set 
> geometry by myself :)
> 
> Anthony Fieroni wrote:
> ^ No, extend geometry with decoration size :)
> 
> Thomas Lübking wrote:
> meehhh...
> 
> That means one will have to filter QEvent::Close of associatedWidget and 
> store the position from there.
> 
> Anthony Fieroni wrote:
> It's strange, about me, associatedWidget has correct frameGeometry even 
> it's hide.
> QObject::connect(KWindowSystem::self(), ::windowRemoved, q, 
> [this](WId winId) {
> if (associatedWidget && associatedWidget->winId() == winId) {
> associatedWidgetPos = 
> associatedWidget->frameGeometry().topLeft();
> }
> });
> Position is correct, but again not work event with Qt::WA_Moved.
> 
> Anthony Fieroni wrote:
> KWindowInfo info(associatedWidget->winId(), NET::WMDesktop | 
> NET::WMGeometry);
> info.geometry().topLeft(); <- decoration is 
> included again
> to filed bug?

You may file a bug, but I frankly doubt your findings.
(Just more or less ported my kwindowsystem tool and the values are correct ;-)

=> How exactly did you determine this assumption? (Just from the walking 
position?)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93143
---


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Store position of widget before hide it
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp cf3e7b5 
>   src/kstatusnotifieritemprivate_p.h 8fdfd4c 
> 
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
> 
> 
> Testing
> ---
> 
> Tested on pixel ration = 1
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

> On March 3, 2016, 10:16 p.m., Thomas Lübking wrote:
> > src/kstatusnotifieritem.cpp, line 934
> > <https://git.reviewboard.kde.org/r/127216/diff/2/?file=446044#file446044line934>
> >
> > append
> > associatedWidget->setAttribute(Qt::WA_Moved);
> > 
> > 
> > 
> > This should tell Qt to demand an explicit position and skip placement 
> > by the WM (yes, this generally works in KWin ;-)
> > 
> > However, this flag should be implicitly set by ::move() unless the 
> > point is invalid, so you might want to "qDebug() << associatedWidgetPos;" 
> > to check whether it's invalid for the failing clients.
> 
> Anthony Fieroni wrote:
> Fun facts :) void KStatusNotifierItemPrivate::minimizeRestore(bool) is 
> called only when you click on tray icon i.e. if i close app with big red X 
> it's not called => i can't store position :)
> 
> Anthony Fieroni wrote:
> More fun facts :) 
> associatedWidget->setGeometry(associatedWidget->frameGeometry()); works in 
> any case ! I will add path if you want, but you will not be happy to set 
> geometry by myself :)
> 
> Anthony Fieroni wrote:
> ^ No, extend geometry with decoration size :)

meehhh...

That means one will have to filter QEvent::Close of associatedWidget and store 
the position from there.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93143
---


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Store position of widget before hide it
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp cf3e7b5 
>   src/kstatusnotifieritemprivate_p.h 8fdfd4c 
> 
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
> 
> 
> Testing
> ---
> 
> Tested on pixel ration = 1
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93143
---




src/kstatusnotifieritem.cpp (line 933)
<https://git.reviewboard.kde.org/r/127216/#comment63504>

append
associatedWidget->setAttribute(Qt::WA_Moved);

This should tell Qt to demand an explicit position and skip placement by 
the WM (yes, this generally works in KWin ;-)

However, this flag should be implicitly set by ::move() unless the point is 
invalid, so you might want to "qDebug() << associatedWidgetPos;" to check 
whether it's invalid for the failing clients.


- Thomas Lübking


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Store position of widget before hide it
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp cf3e7b5 
>   src/kstatusnotifieritemprivate_p.h 8fdfd4c 
> 
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
> 
> 
> Testing
> ---
> 
> Tested on pixel ration = 1
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

> On March 3, 2016, 8:57 p.m., Anthony Fieroni wrote:
> > Ping
> 
> Martin Klapetek wrote:
> Does not work for Quassel and Konversation, I'd like to know why before I 
> approve your patch.
> 
> Martin Klapetek wrote:
> Ah, interestingly I've never received the replies you made above. Must 
> have ended in some @kde.org filter or something.

Didn't receive them either.
Either Anthony forgot to push the "publish" button (happens ;-) or kde.org 
took a nap.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93128
---


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Store position of widget before hide it
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp cf3e7b5 
>   src/kstatusnotifieritemprivate_p.h 8fdfd4c 
> 
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
> 
> 
> Testing
> ---
> 
> Tested on pixel ration = 1
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on X11

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review92888
---



just store the widget/window position when hiding the window (ie. in the else 
branch) and avoid walkign across the window system to figure the restore 
position.

The patch will fail miserably if there's no WM, the SNI window becomes 
unmanaged (again) and half a dozen other occasions.

- Thomas Lübking


On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> ---
> 
> (Updated Feb. 29, 2016, 5:18 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin 
> Klapetek.
> 
> 
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> I add specific X11 code till we have proper move in KWindowSystem
> 
> 
> Diffs
> -
> 
>   src/kstatusnotifieritem.cpp cf3e7b5 
> 
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
> 
> 
> Testing
> ---
> 
> Tested on pixel ration = 1
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127191: KCompletionBox should *not* be a tooltip

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127191/#review92809
---




src/kcompletionbox.cpp (line 66)
<https://git.reviewboard.kde.org/r/127191/#comment63235>

q->setAttribute(Qt::WA_X11NetWmWindowTypeCombo); // broken??


- Thomas Lübking


On Feb. 26, 2016, 1:23 p.m., Marco Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127191/
> ---
> 
> (Updated Feb. 26, 2016, 1:23 p.m.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> ---
> 
> KCompletionbox it's actually more of  a combobox popup than a tooltip.
> this sets it the proper type (unfortunately with an hack due the type not 
> being available to Qt::WindowFlags)
> 
> without this, the new morphingpopups KWin effect will animate completion 
> boxes (such as in file open dialog, kate, dolphin adress bar etc)
> 
> 
> Diffs
> -
> 
>   src/kcompletionbox.cpp 99afc8e 
> 
> Diff: https://git.reviewboard.kde.org/r/127191/diff/
> 
> 
> Testing
> ---
> 
> Same behavior, due to the Window and bypasswindowmanager that are those two 
> that made the desired behavior when tooltip was picked
> 
> 
> Thanks,
> 
> Marco Martin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/
---

(Updated Jan. 15, 2016, 9:16 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, kwin and Albert Astals Cid.


Changes
---

Submitted with commit 46c525f8fe77a10923eb2d604bf0f71bf59b5d1d by Thomas 
Lübking to branch master.


Bugs: 354811
https://bugs.kde.org/show_bug.cgi?id=354811


Repository: kwindowsystem


Description
---

summarized, alternative to https://git.reviewboard.kde.org/r/126397/

NOTICE: this compiles but is otherwise *completely* untested!


Diffs
-

  src/platforms/xcb/kwindowsystem.cpp 9d28704 

Diff: https://git.reviewboard.kde.org/r/126403/diff/


Testing
---

Albert performed a successful test on the bug.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

> On Jan. 10, 2016, 11:33 p.m., David Faure wrote:
> > src/platforms/xcb/kwindowsystem.cpp, line 80
> > <https://git.reviewboard.kde.org/r/126403/diff/3/?file=430337#file430337line80>
> >
> > this ';' seems unnecessary and weird, it's the end of an if() block.

Stray junk indeed.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/#review90863
---


On Jan. 10, 2016, 9:58 p.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Jan. 10, 2016, 9:58 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> Albert performed a successful test on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/
---

(Updated Jan. 10, 2016, 9:58 p.m.)


Review request for KDE Frameworks, kwin and Albert Astals Cid.


Bugs: 354811
https://bugs.kde.org/show_bug.cgi?id=354811


Repository: kwindowsystem


Description
---

summarized, alternative to https://git.reviewboard.kde.org/r/126397/

NOTICE: this compiles but is otherwise *completely* untested!


Diffs (updated)
-

  src/platforms/xcb/kwindowsystem.cpp 9d28704 

Diff: https://git.reviewboard.kde.org/r/126403/diff/


Testing
---

Albert performed a successful test on the bug.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

> On Jan. 10, 2016, 9:55 a.m., David Faure wrote:
> > src/platforms/xcb/kwindowsystem.cpp, line 61
> > <https://git.reviewboard.kde.org/r/126403/diff/2/?file=428224#file428224line61>
> >
> > How does this work? I see code iterating through that list, but I don't 
> > see code adding to the list in the first place. Dead code? Unfinished code?

Forgot to add them to the list ...


> On Jan. 10, 2016, 9:55 a.m., David Faure wrote:
> > src/platforms/xcb/kwindowsystem.cpp, line 69
> > <https://git.reviewboard.kde.org/r/126403/diff/2/?file=428224#file428224line69>
> >
> > This reads weird, (casting from GuiApp to GuiApp), but in fact it's 
> > because there's no QGuiApplication::instance, it's 
> > QCoreApplication::instance, I would suggest to use that to make this more 
> > readable.
> > 
> > qApp should work too. It expands to exactly that, when #including 
> > QGuiApplication.

I've no idea how that ended up there. Seriously not.


On Jan. 10, 2016, 9:55 a.m., Thomas Lübking wrote:
> > The commit log also needs to be updated, assuming it's still equal to the 
> > description that is visible in ReviewBoard.

No, it's just "get rid of QApplication dependency" and commit hooks.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/#review90834
---


On Jan. 6, 2016, 5:34 p.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Jan. 6, 2016, 5:34 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> Albert performed a successful test on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/
---

(Updated Jan. 6, 2016, 5:34 p.m.)


Review request for KDE Frameworks, kwin and Albert Astals Cid.


Bugs: 354811
https://bugs.kde.org/show_bug.cgi?id=354811


Repository: kwindowsystem


Description
---

summarized, alternative to https://git.reviewboard.kde.org/r/126397/

NOTICE: this compiles but is otherwise *completely* untested!


Diffs
-

  src/platforms/xcb/kwindowsystem.cpp 9d28704 

Diff: https://git.reviewboard.kde.org/r/126403/diff/


Testing (updated)
---

Albert performed a successful test on the bug.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

> On Jan. 4, 2016, 7:42 a.m., Martin Gräßlin wrote:
> > anyone tested this on an "affected system"?
> 
> Albert Astals Cid wrote:
> I have not, I thought you had made it clear in the past you thought it 
> was a bad idea since it was all supposed to be widget based anyway.
> 
> Thomas Lübking wrote:
> We're trying to resolve this ;-)
> 
> KWindowSystem could otherwise never be used by QGuiApplication's
> As bonus, we can probably unlink Qt6Widgets in "it's certainly not called 
> KF6 - that would be far too simple" :-P
> (I'm not sure whether we can/should that right now - while you should 
> explicitly link stuff you need, we don't live in a should-land)
> 
> => If you can, give it a try (compiz isn't provided by my distro)
> 
> Albert Astals Cid wrote:
> I can be your tester if you tell me what to test.
> 
> Thomas Lübking wrote:
> Make kscreenlocker_greet crash when using compiz as WM, see bug #354811
> 
> Albert Astals Cid wrote:
> Running /usr/lib/x86_64-linux-gnu/libexec/kscreenlocker_greet on Ubuntu 
> Xenial under Unity7 crashes before applying this patch to kwindowsystem and 
> seems to work fine after applying the patch.

That's the desired result, many thanks for your effort.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/#review90522
---


On Jan. 2, 2016, 9:57 a.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Jan. 2, 2016, 9:57 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> no, not the least. esp. not on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118909: Imply SkipTaskbar and SkipPager depending on Window Type

[Thomas Lübking]

> On June 23, 2014, 8:08 p.m., Thomas Lübking wrote:
> > What about (non override) utility windows (eg. assume Qt would not set 
> > floating dock override to bring its own titlebar)?
> > While kwin (iirc by default) hides them for inactive windows, that's rather 
> > a feature.
> > 
> > -> By their nature, they should not be in the taskbar. But should they not 
> > appear in pagers?
> 
> Martin Gräßlin wrote:
> > What about (non override) utility windows
> 
> Eike, what's your opinion on that from task manager perspective? Is the 
> nature of a utility window to skip taskbar?
> 
> > But should they not appear in pagers?
> 
> yes, it has the same text: "This hint should be requested by the 
> application, i.e. it indicates that the window by nature is never in the 
> Pager."
> 
> Eike Hein wrote:
> I totally missed this review somehow. I feel like Utility windows should 
> skip, yeah.
> 
> See also plasma-workspace.git c34550cf.
> 
> Can you recheck you still want this? You can get a +1 then.

I guess we all actually agree on skipping the *taskbar*, the question was about 
pagers.
They're distinct hints - so the nature of one does hardly imply the nature of 
the other.

No idea how widely pagers are used, but it would hide eg. the gimp docks and we 
do not enforce them to be on the VD of the image editor (we would not even if 
they were transient...), ie. they'd loose their representation in the pager 
(and you don't know they're over there)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118909/#review60843
---


On June 23, 2014, 6:50 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118909/
> ---
> 
> (Updated June 23, 2014, 6:50 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Eike Hein.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> Imply SkipTaskbar and SkipPager depending on Window Type
> 
> Quoting EWMH:
> "Applications should not set this hint if _NET_WM_WINDOW_TYPE already
> conveys the exact nature of the window."
> 
> This means if the window type implies a SkipPager or SkipTaskbar the
> window will not have it set. To simplify the life of our API users we
> should add the state and not require our users to check that manually.
> 
> Every Normal or Dialog window does not imply that it shouldn't be shown.
> As our KDE Override is not a proper window type we can assume that it's
> also a Normal window or a Dialog.
> 
> The change is done in KWindowInfo and requires that the API user adds
> NET::WMWindowType explicitly to the properties. NETWinInfo is not
> adjusted to have it reflect the actual state of the atoms. Here we can
> assume that the users of the more low-level API are aware of the EWMH
> spec and will implement these checks themselves.
> 
> 
> Diffs
> -
> 
>   autotests/kwindowinfox11test.cpp 50ce806add5ea8f6cb19e537609e936c3d0275bd 
>   src/kwindowinfo.h e9b7a0af66a1e0e0c4e4b84496157dd53035abc8 
>   src/kwindowinfo_x11.cpp 87a887c7e068f71cffc58c184bcd4b4722564047 
> 
> Diff: https://git.reviewboard.kde.org/r/118909/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

> On Jan. 4, 2016, 7:42 a.m., Martin Gräßlin wrote:
> > anyone tested this on an "affected system"?
> 
> Albert Astals Cid wrote:
> I have not, I thought you had made it clear in the past you thought it 
> was a bad idea since it was all supposed to be widget based anyway.
> 
> Thomas Lübking wrote:
> We're trying to resolve this ;-)
> 
> KWindowSystem could otherwise never be used by QGuiApplication's
> As bonus, we can probably unlink Qt6Widgets in "it's certainly not called 
> KF6 - that would be far too simple" :-P
> (I'm not sure whether we can/should that right now - while you should 
> explicitly link stuff you need, we don't live in a should-land)
> 
> => If you can, give it a try (compiz isn't provided by my distro)
> 
> Albert Astals Cid wrote:
> I can be your tester if you tell me what to test.

Make kscreenlocker_greet crash when using compiz as WM, see bug #354811


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/#review90522
-------


On Jan. 2, 2016, 9:57 a.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Jan. 2, 2016, 9:57 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> no, not the least. esp. not on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/
---

(Updated Jan. 2, 2016, 9:57 a.m.)


Review request for KDE Frameworks, kwin and Albert Astals Cid.


Bugs: 354811
https://bugs.kde.org/show_bug.cgi?id=354811


Repository: kwindowsystem


Description
---

summarized, alternative to https://git.reviewboard.kde.org/r/126397/

NOTICE: this compiles but is otherwise *completely* untested!


Diffs (updated)
-

  src/platforms/xcb/kwindowsystem.cpp 9d28704 

Diff: https://git.reviewboard.kde.org/r/126403/diff/


Testing
---

no, not the least. esp. not on the bug.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126403: Get rid of QApplication dependency

[Thomas Lübking]

> On Jan. 1, 2016, 7:47 p.m., David Faure wrote:
> > src/platforms/xcb/kwindowsystem.cpp, line 891
> > <https://git.reviewboard.kde.org/r/126403/diff/1/?file=424255#file424255line891>
> >
> > isn't that "return displayGeometry()?"
> > 
> > If not, then at least something like QRect(QPoint(0,0), 
> > displayGeometry().size()) (encapsulated in a convenience function) would 
> > avoid calling displayGeometry() twice.

Yes, is. Patch evolution had displayWidth/Height from the root window before I 
figured we'd better clone the combined screen area approach. Thanks for the 
catch.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126403/#review90395
-------


On Dec. 17, 2015, 3:38 p.m., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126403/
> ---
> 
> (Updated Dec. 17, 2015, 3:38 p.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> summarized, alternative to https://git.reviewboard.kde.org/r/126397/
> 
> NOTICE: this compiles but is otherwise *completely* untested!
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 9d28704 
> 
> Diff: https://git.reviewboard.kde.org/r/126403/diff/
> 
> 
> Testing
> ---
> 
> no, not the least. esp. not on the bug.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256>
> >
> > This patch looks wrong because KPushButton can be used outside of 
> > "dialog button boxes", while the styleHint is supposed to be only about 
> > dialog button boxes.
> > 
> > QPushButton::sizeHint does this:
> > bool showButtonBoxIcons = 
> > qobject_cast<QDialogButtonBox*>(parentWidget())
> >   && 
> > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
> > which is a solution for testing the parent widget.
> > 
> > I still don't fully understand the issue though, at painting time both 
> > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
> > these two would be working differently.
> > Is this a workaround for KPushButton which doesn't fix QPushButton?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the sty

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256>
> >
> > This patch looks wrong because KPushButton can be used outside of 
> > "dialog button boxes", while the styleHint is supposed to be only about 
> > dialog button boxes.
> > 
> > QPushButton::sizeHint does this:
> > bool showButtonBoxIcons = 
> > qobject_cast<QDialogButtonBox*>(parentWidget())
> >   && 
> > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
> > which is a solution for testing the parent widget.
> > 
> > I still don't fully understand the issue though, at painting time both 
> > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
> > these two would be working differently.
> > Is this a workaround for KPushButton which doesn't fix QPushButton?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the sty

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256>
> >
> > This patch looks wrong because KPushButton can be used outside of 
> > "dialog button boxes", while the styleHint is supposed to be only about 
> > dialog button boxes.
> > 
> > QPushButton::sizeHint does this:
> > bool showButtonBoxIcons = 
> > qobject_cast<QDialogButtonBox*>(parentWidget())
> >   && 
> > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
> > which is a solution for testing the parent widget.
> > 
> > I still don't fully understand the issue though, at painting time both 
> > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
> > these two would be working differently.
> > Is this a workaround for KPushButton which doesn't fix QPushButton?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the sty

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > 
> >
> > This patch looks wrong because KPushButton can be used outside of 
> > "dialog button boxes", while the styleHint is supposed to be only about 
> > dialog button boxes.
> > 
> > QPushButton::sizeHint does this:
> > bool showButtonBoxIcons = 
> > qobject_cast(parentWidget())
> >   && 
> > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
> > which is a solution for testing the parent widget.
> > 
> > I still don't fully understand the issue though, at painting time both 
> > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
> > these two would be working differently.
> > Is this a workaround for KPushButton which doesn't fix QPushButton?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?

The "problem" is that this is really scattered around everywhere :(

One major catch should be (frameworksintegration)

```
diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
b/src/platformtheme/kdeplatformfiledialoghelper.cpp
index 11e7efb..9cd374c 100644
--- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
+++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
@@ -161,6 +161,11 @@ void 
KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
 m_fileWidget->setMode(KFile::File);
 break;
 }
+// ::setOperationMode happily adds icons to our buttonbox, so we clear 
them everytime the mode is set
+if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
nullptr, this)) {
+foreach (QAbstractButton *button, m_buttons->buttons())
+button->setIcon(QIcon());
+}
 }
 
 void KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel 
label, const QString )
diff --git a/src/platformtheme/kdirselectdialog.cpp 
b/src/platformtheme/kdirselectdialog.cpp
index 0a65cd3..13d7dc7 100644
--- a/src/platformtheme/kdirselectdialog.cpp
+++ b/src/platformtheme/kdirselectdialog.cpp
@@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl , 
bool localOnly, QWidget
 m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
QDialogButtonBox::Cancel);
 connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
 connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
+if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
nullptr, this)) {
+foreach (QAbstractButton *button, m_buttons->buttons())
+button->setIcon(QIcon());
+}
 topLayout->addWidget(m_buttons);
 
 QHBoxLayout *hlay = new QHBoxLayout(page);
```

But that somehow doesn't scale.

KGuiItem::apply would have to catch that, but doesn't.
QDialogButtonBox::addButton *could* catch things, but doesn't (also it's not 
clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, so 
QPushButton::setIcon() would have to catch it and QPushButton would have to 
catch it on reparenting.


=> For a complete solution, QPushButton actually needs painting code to check 
the parent & style hint when setting up the style option - or we simply rely on 
the style testing (widget && 
qobject_cast(widget->parentWidget())) when calculating sizes 
and painting the button.
The only alternative is to walk a long way and tell everyone to please check 
the hint and fix their buttonboxes eewww.


==> QPushButton will require a "fix" to handle the StyleHint, but we cannot 
expect *Q*PushButton to honor some global KDE setting, that's really a job for 
the platform integration and in this case ultimately the style.


=> the platform integration plugin (KDE part in all Qt applications) needs to 
read that setting and expose it to the styles.
Alternatively the styles could read the setting from kdeglobals directly, but 
that requires them to link kconfig (to do it correctly, just grabbing it from 
the users kdeglobals ini isn't that much of a problem ;-), ie. rules out 
Qt-only styles (and somehow contrasts w/ the KF5 approach to things)


Ideally™ there would be an official "Qt::AA_PushButtonTextOnly" (rename me) 
flag for the QPA to set and QPushButtons to pick.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89737
---


On Dec. 11, 2015, 4:26 p.m., 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256>
> >
> > This patch looks wrong because KPushButton can be used outside of 
> > "dialog button boxes", while the styleHint is supposed to be only about 
> > dialog button boxes.
> > 
> > QPushButton::sizeHint does this:
> > bool showButtonBoxIcons = 
> > qobject_cast<QDialogButtonBox*>(parentWidget())
> >   && 
> > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
> > which is a solution for testing the parent widget.
> > 
> > I still don't fully understand the issue though, at painting time both 
> > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
> > these two would be working differently.
> > Is this a workaround for KPushButton which doesn't fix QPushButton?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the sty

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 19, 2015, 10:23 a.m., David Faure wrote:
> > src/kdeui/kpushbutton.cpp, line 256
> > <https://git.reviewboard.kde.org/r/126308/diff/6/?file=421676#file421676line256>
> >
> > This patch looks wrong because KPushButton can be used outside of 
> > "dialog button boxes", while the styleHint is supposed to be only about 
> > dialog button boxes.
> > 
> > QPushButton::sizeHint does this:
> > bool showButtonBoxIcons = 
> > qobject_cast<QDialogButtonBox*>(parentWidget())
> >   && 
> > style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons);
> > which is a solution for testing the parent widget.
> > 
> > I still don't fully understand the issue though, at painting time both 
> > QPushButton and KPushButton call QStyle's CE_PushButton, so I don't see why 
> > these two would be working differently.
> > Is this a workaround for KPushButton which doesn't fix QPushButton?
> 
> René J.V. Bertin wrote:
> David, I'm dropping this whole RR, leaving it open only in case Thomas 
> wants to pursue his view on fixing what there is to fix.
> 
> You are right though, it's a workaround for KPushButton which doesn't 
> depend on fixing QPushButton. And whether or not QPushButton requires fixing 
> is apparently the big question. What is your idea on the scope of 
> `ShowIconsOnButtons`?
> 
> Thomas Lübking wrote:
> The "problem" is that this is really scattered around everywhere :(
> 
> One major catch should be (frameworksintegration)
> 
> ```
> diff --git a/src/platformtheme/kdeplatformfiledialoghelper.cpp 
> b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> index 11e7efb..9cd374c 100644
> --- a/src/platformtheme/kdeplatformfiledialoghelper.cpp
> +++ b/src/platformtheme/kdeplatformfiledialoghelper.cpp
> @@ -161,6 +161,11 @@ void 
> KDEPlatformFileDialog::setFileMode(QFileDialogOptions::FileMode mode)
>  m_fileWidget->setMode(KFile::File);
>  break;
>  }
> +// ::setOperationMode happily adds icons to our buttonbox, so we 
> clear them everytime the mode is set
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  }
>  
>  void 
> KDEPlatformFileDialog::setCustomLabel(QFileDialogOptions::DialogLabel label, 
> const QString )
> diff --git a/src/platformtheme/kdirselectdialog.cpp 
> b/src/platformtheme/kdirselectdialog.cpp
> index 0a65cd3..13d7dc7 100644
> --- a/src/platformtheme/kdirselectdialog.cpp
> +++ b/src/platformtheme/kdirselectdialog.cpp
> @@ -299,6 +299,10 @@ KDirSelectDialog::KDirSelectDialog(const QUrl 
> , bool localOnly, QWidget
>  m_buttons->setStandardButtons(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel);
>  connect(m_buttons, SIGNAL(accepted()), this, SLOT(accept()));
>  connect(m_buttons, SIGNAL(rejected()), this, SLOT(reject()));
> +if (!style()->styleHint(QStyle::SH_DialogButtonBox_ButtonsHaveIcons, 
> nullptr, this)) {
> +foreach (QAbstractButton *button, m_buttons->buttons())
> +button->setIcon(QIcon());
> +}
>  topLayout->addWidget(m_buttons);
>  
>  QHBoxLayout *hlay = new QHBoxLayout(page);
> ```
> 
> But that somehow doesn't scale.
> 
> KGuiItem::apply would have to catch that, but doesn't.
> QDialogButtonBox::addButton *could* catch things, but doesn't (also it's 
> not clear whether or when m_buttons->button(SomeRole)->setIcon() is invoked, 
> so QPushButton::setIcon() would have to catch it and QPushButton would have 
> to catch it on reparenting.
> 
> 
> => For a complete solution, QPushButton actually needs painting code to 
> check the parent & style hint when setting up the style option - or we simply 
> rely on the style testing (widget && 
> qobject_cast<QDialogButtonBox*>(widget->parentWidget())) when calculating 
> sizes and painting the button.
> The only alternative is to walk a long way and tell everyone to please 
> check the hint and fix their buttonboxes eewww.
> 
> 
> 
> ==> QPushButton will require a "fix" to handle the StyleHint, but we 
> cannot expect *Q*PushButton to honor some global KDE setting, that's really a 
> job for the platform integration and in this case ultimately the sty

Re: Review Request 126397: [xcb] Safety check whether we have a QApplication in mapViewport

[Thomas Lübking]

> On Dec. 17, 2015, 12:55 p.m., Thomas Lübking wrote:
> > Looks like this boils down to multiple qApp->desktop()->size() calls, ie. 
> > displayWidth/displayHeight in kwinglobals.h, right?
> > 
> > We could just "borrow" that code and kick the qApp dep then?

Ok, further check - there's another, unrelated, trap when trying to access the 
root winId, we'd better fetch that from QX11Info


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126397/#review89648
---


On Dec. 17, 2015, 9:21 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126397/
> ---
> 
> (Updated Dec. 17, 2015, 9:21 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> We observed that with Compiz as window manager various applications
> crash if they use QGuiApplication instead of QApplication. The reason
> for this is that on Compiz the mapViewport code paths are used and
> that has a widgets dependency.
> 
> This change should ensure that applications do at least not crash
> in this condition.
> 
> BUG: 354811
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 
> 9d287043c24894ca3c29c439c7939b139da055e8 
> 
> Diff: https://git.reviewboard.kde.org/r/126397/diff/
> 
> 
> Testing
> ---
> 
> User in referenced bug report tested it, works.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126397: [xcb] Safety check whether we have a QApplication in mapViewport

[Thomas Lübking]

> On Dec. 17, 2015, 12:55 p.m., Thomas Lübking wrote:
> > Looks like this boils down to multiple qApp->desktop()->size() calls, ie. 
> > displayWidth/displayHeight in kwinglobals.h, right?
> > 
> > We could just "borrow" that code and kick the qApp dep then?
> 
> Thomas Lübking wrote:
> Ok, further check - there's another, unrelated, trap when trying to 
> access the root winId, we'd better fetch that from QX11Info
> 
> Martin Gräßlin wrote:
> > Looks like this boils down to multiple qApp->desktop()->size() calls, 
> ie. displayWidth/displayHeight in kwinglobals.h, right?
> 
> I'm not sure what QDesktopWidget really does. If it's just the root 
> window size: yes sure.

Checked it - the geometry is the bounding rect of all QScreens.

Blast, we'll have to use that as well since we've geometry() uses and 
unfortunately cannot just assume that this starts as 0,0 :-(


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126397/#review89648
---


On Dec. 17, 2015, 9:21 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126397/
> ---
> 
> (Updated Dec. 17, 2015, 9:21 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> We observed that with Compiz as window manager various applications
> crash if they use QGuiApplication instead of QApplication. The reason
> for this is that on Compiz the mapViewport code paths are used and
> that has a widgets dependency.
> 
> This change should ensure that applications do at least not crash
> in this condition.
> 
> BUG: 354811
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 
> 9d287043c24894ca3c29c439c7939b139da055e8 
> 
> Diff: https://git.reviewboard.kde.org/r/126397/diff/
> 
> 
> Testing
> ---
> 
> User in referenced bug report tested it, works.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126397: [xcb] Safety check whether we have a QApplication in mapViewport

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126397/#review89648
---


Looks like this boils down to multiple qApp->desktop()->size() calls, ie. 
displayWidth/displayHeight in kwinglobals.h, right?

We could just "borrow" that code and kick the qApp dep then?

- Thomas Lübking


On Dec. 17, 2015, 9:21 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126397/
> ---
> 
> (Updated Dec. 17, 2015, 9:21 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> We observed that with Compiz as window manager various applications
> crash if they use QGuiApplication instead of QApplication. The reason
> for this is that on Compiz the mapViewport code paths are used and
> that has a widgets dependency.
> 
> This change should ensure that applications do at least not crash
> in this condition.
> 
> BUG: 354811
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 
> 9d287043c24894ca3c29c439c7939b139da055e8 
> 
> Diff: https://git.reviewboard.kde.org/r/126397/diff/
> 
> 
> Testing
> ---
> 
> User in referenced bug report tested it, works.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line39>
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.
> 
> René J.V. Bertin wrote:
> No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that 
> "does correctly" is the one that takes a StandardButton. I haven't had time 
> to test this (will need to rebuild QtBase first) but I'm pretty sure that 
> that method creates a button with an icon with its sequence
> 
> ```
> QPushButton *button = new QPushButton(text, this);
> d->addButton(button, role);
> ```
> 
> My approach here is to avoid adding an icon if ButtonsHaveIcons is false, 
> or remove the icon if one was already added. That's what QDB does with its 
> ::addButton(StandardButton btn) method (calling a private createButton() 
> method). Any other approach is useless without a style supporting and 
> enforcing ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't 
> need to be fixed in the first place...
> 
> Thomas Lübking wrote:
> ::addButton(text, role) creates "new QPushButton(text, this)" - those 
> should seriously not have any icons.
> 
> > whith such a style KDialogButtonBox doesn't need to be fixed in the 
> first place
> 
> If it's broken, it needs to be fixed - you cannot bail out with "the 
> style is correcting that for us" (I've been fixing far too many kdelibs/qtgui 
> bugs in the style ;-)
> 
> René J.V. Bertin wrote:
> > ::addButton(text, role) creates "new QPushButton(text, this)" - those 
> should seriously not have any icons.
> 
> Agreed, they shouldn't *show* any if the user doesn't want to see them. 
> AFAIC they can have a whole bunch of icons as long as they're not displayed. 
> 
> This argument is a bit useless as long as we don't know if an interface 
> should stop showing icons the moment the user unticks the corresponding 
> setting in systemsettings (and start showing them again when the setting is 
> ticked). If it's ok to tell the user that "the new setting will only be 
> respected after an application restart", then fine, let's simply not add 
> icons when they're not wanted. In all those countless places where the 
> setting will have to be applied.
> 
> But didn't you point out yourself that the style is in the best position 
> to avoid drawing any unwanted icons?

If you create a PushButton with this constructor, the button has at this point 
no icon assigned. This has absolutely nothing to do with some setting or the 
display of anything - where should the button have obtained an icon? None is 
set here in the first place. QPushButton::icon().isNull()


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line57>
> >
> > you can completely spare this, there's no reason to manipulate a copy 
> > of the GuiItem, just burns CPU
> 
> René J.V. Bertin wrote:
> In that case I'll have to remove the `const` from guiitem, meaning a 
> change to the API.
> 
> Thomas Lübking wrote:
> No, you do not touch the KGuiItem that comes (it's not yours!) and it's 
> pointless to create a copy, strip the icon from that, assign it to the button 
> (and maybe even strip the icon from the button) - you just apply the GuiItem 
> that enters and strip the icon from the button. The interim (deep) GuiItem 
> copy is just detour in the path to remove the icon from the button.
> 
> René J.V. Bertin wrote:
> I must be getting on with other real-life stuff now; I agree with not 
> touching the incoming item of course. I'll see if there isn't a way to avoid 
> adding the unwanted icon at all, to avoid the icon deep copy as well 
> (probably the most expensive part of a GuiItem deep copy, no?)

There's no deep copy of an icon, it's implicitly shared. And if there was, 
copying the KGuiItem would still only make things worse ...


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 61
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line61>
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)
> 
> René J.V. Bertin wrote:
> Maybe it won't leak, but the question remains why what buttons with an 
> invalid role a

Re: Review Request 126397: [xcb] Safety check whether we have a QApplication in mapViewport

[Thomas Lübking]

> On Dec. 17, 2015, 12:55 p.m., Thomas Lübking wrote:
> > Looks like this boils down to multiple qApp->desktop()->size() calls, ie. 
> > displayWidth/displayHeight in kwinglobals.h, right?
> > 
> > We could just "borrow" that code and kick the qApp dep then?
> 
> Thomas Lübking wrote:
> Ok, further check - there's another, unrelated, trap when trying to 
> access the root winId, we'd better fetch that from QX11Info
> 
> Martin Gräßlin wrote:
> > Looks like this boils down to multiple qApp->desktop()->size() calls, 
> ie. displayWidth/displayHeight in kwinglobals.h, right?
> 
> I'm not sure what QDesktopWidget really does. If it's just the root 
> window size: yes sure.
> 
> Thomas Lübking wrote:
> Checked it - the geometry is the bounding rect of all QScreens.
> 
> Blast, we'll have to use that as well since we've geometry() uses and 
> unfortunately cannot just assume that this starts as 0,0 :-(

https://git.reviewboard.kde.org/r/126403/


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126397/#review89648
---


On Dec. 17, 2015, 9:21 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126397/
> ---
> 
> (Updated Dec. 17, 2015, 9:21 a.m.)
> 
> 
> Review request for KDE Frameworks, kwin and Albert Astals Cid.
> 
> 
> Bugs: 354811
> https://bugs.kde.org/show_bug.cgi?id=354811
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> We observed that with Compiz as window manager various applications
> crash if they use QGuiApplication instead of QApplication. The reason
> for this is that on Compiz the mapViewport code paths are used and
> that has a widgets dependency.
> 
> This change should ensure that applications do at least not crash
> in this condition.
> 
> BUG: 354811
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/kwindowsystem.cpp 
> 9d287043c24894ca3c29c439c7939b139da055e8 
> 
> Diff: https://git.reviewboard.kde.org/r/126397/diff/
> 
> 
> Testing
> ---
> 
> User in referenced bug report tested it, works.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 39
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line39>
> >
> > QDialogButtonBox::addButton should do correctly anyway, so please don't 
> > workaround things that are not broken.
> 
> René J.V. Bertin wrote:
> No, I've looked at Qt 5.5.1 . The only QDialogButtonBox::addButton that 
> "does correctly" is the one that takes a StandardButton. I haven't had time 
> to test this (will need to rebuild QtBase first) but I'm pretty sure that 
> that method creates a button with an icon with its sequence
> 
> ```
> QPushButton *button = new QPushButton(text, this);
> d->addButton(button, role);
> ```
> 
> My approach here is to avoid adding an icon if ButtonsHaveIcons is false, 
> or remove the icon if one was already added. That's what QDB does with its 
> ::addButton(StandardButton btn) method (calling a private createButton() 
> method). Any other approach is useless without a style supporting and 
> enforcing ButtonsHaveIcons, but which such a style KDialogButtonBox doesn't 
> need to be fixed in the first place...

::addButton(text, role) creates "new QPushButton(text, this)" - those should 
seriously not have any icons.

> whith such a style KDialogButtonBox doesn't need to be fixed in the first 
> place

If it's broken, it needs to be fixed - you cannot bail out with "the style is 
correcting that for us" (I've been fixing far too many kdelibs/qtgui bugs in 
the style ;-)


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 57
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line57>
> >
> > you can completely spare this, there's no reason to manipulate a copy 
> > of the GuiItem, just burns CPU
> 
> René J.V. Bertin wrote:
> In that case I'll have to remove the `const` from guiitem, meaning a 
> change to the API.

No, you do not touch the KGuiItem that comes (it's not yours!) and it's 
pointless to create a copy, strip the icon from that, assign it to the button 
(and maybe even strip the icon from the button) - you just apply the GuiItem 
that enters and strip the icon from the button. The interim (deep) GuiItem copy 
is just detour in the path to remove the icon from the button.


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 61
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line61>
> >
> > unrelated and it won't leak, since the cleanup is done by the 
> > parent/child relation ("this" passed to KPushButton)
> 
> René J.V. Bertin wrote:
> Maybe it won't leak, but the question remains why what buttons with an 
> invalid role are good for.

Did you see such role being used? It would be a client code bug (the symbol is 
juts for completeness sake, so that you can eg. assign it to a role, pipe that 
to some transformation functions, check whether it's still invalid and then 
react to that)


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 70
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#file421661line70>
> >
> > Setting the icon is sufficient, please do not mess around with other 
> > attributes.
> 
> René J.V. Bertin wrote:
> Are you sure? setIcon() doesn't call setIconSize() nor does it reset any 
> size information already present. Is it a good idea to replace an icon and 
> leaving the size information from the previous icon)? NB: should the icon 
> from the KGuiItem override the role's standard icon or should it be the other 
> way round (provided icon as a default when the role doesn't provide an icon, 
> for instance)?

setIcon *shall* not setIconSize, the two values are completely orhtogonal. 
Dropping the size information will just get you into trouble when you should 
require it again.
If eg. a style would reserve icon space of iconSize despite there actually is 
no icon to paint, the style is simply broken.

The icon size refers to the wanted size in the widget, not what the icon 
provides. Resolving that is job of the icon loader (or painting routine, eg. 
the style)

About GuiItem ./. StdRole: FIFO, ie. the last setter should usually win (if 
you've a button with a dedicated icon and role "ok" and switch the role to 
"delete", the "ok" icon is most certainly no longer correct ;-)


> On Dec. 11, 2015, 1:55 p.m., Thomas Lübking wrote:
> > src/kdeui/kdialogbuttonbox.cpp, line 75
> > <https://git.reviewboard.kde.org/r/126308/diff/5/?file=421661#fil

Re: "Buttons have icons", round N+1

[Thomas Lübking]

On Sonntag, 13. Dezember 2015 16:20:19 CEST, René J.V. wrote:

There has been some discussion around one of my RRs concerning 
how to respect KDE's ShowIconsOnPushButtons and Qt's 
SH_DialogButtonBox_ButtonsHaveIcons style hint.


The ultimate way would be in the style that does the actual 
drawing, but that is probably not the one with the smallest 
overhead.


I object the overhead concern. The global solution is easily applied by the 
style (the icon isn't loaded from disk or rendered until you/the style requires 
a pixmap from painting - until then there's just a string that hints which icon 
to use)


The dialog in question is a KMessageBox with a few KStandardGuiItems.
In a comparable situation, Qt's own code simply doesn't add the 
standard icons to the buttons being created, when 
SH_DialogButtonBox_ButtonsHaveIcons is false.


KMessageBox isn't a QDialogButtonBox


No, but it uses one - the problem is the same as in the kdelibs4support code of 
KDialogButtonBox - the assignment of the KGuiItem which ignores 
SH_DialogButtonBox_ButtonsHaveIcons in all the *Internal functions of 
KMessageBox. Needs a similar fix (to respect the hint)

So this is as well orthogonal to the general Pushbutton 
icon/ShowIconsOnPushButtons question (but simply a bug that needs to be fixed)

It might seem reasonable to query the parent widget of the button in KGuiItem to catch a 
variety of occasions at once (though this doesn't guarantee anything, parent and style 
would have to be tracked to make this "perfect" or QPushButton would check its 
parent widget on painting and conditionally omit the icon when it's a buttonbox and the 
style doesn't want icons there)

Cheers,
Thomas
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms
> 
> Thomas Lübking wrote:
> > 1: why should one care?
> 
> Because, as explained, that is what the hint says. Nothing else.
> 
> > I have found no sign in the code that the ShowIconsOnPushButtons hint 
> is to be used only for dialogs
> 
> No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
> KPushButton.
> ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but 
> NOT vv.
> 
> 

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126308/#review89321
---


1. What tells you that this is a dialog buttonbox pushbutton?
2. What happens if the button has no text?


The bug is in QDialogButtonBox (or rather the K variant, 
QDialogButtonBoxPrivate::createButton() seems to incorporate the hint correctly)


[KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was interpreted by 
KPushButton _and_ kstyle for the hint, but the hint only covers 
Q'DialogButtonBox'es - there's simply no global rule for this like 
AA_DontShowIconsInMenus

=> KDialogButtonBox shall respect the hint for its buttons (there're two 
special creation routines).

For the rest, the platform plugin ideally picks the kdeglobals setting and 
exports it to the application object (dyn property?) where the style can pick 
it and incorporate it into its calculations (ie. if no icons are wanted and 
there's text or arrow, omit the icon in size calculation and painting)

"Fixing" that in deprecated KPushButton doesn't really fix anything. We'll face 
the mix we had, just that users of QPushButton were far less prone to pass them 
an icon in pre-KF5 times.

Please also attach Hugo Pereira Da Costa.

- Thomas Lübking


On Dec. 10, 2015, 9:01 p.m., René J.V. Bertin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126308/
> ---
> 
> (Updated Dec. 10, 2015, 9:01 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks and Qt KDE.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> KF5 applications have long had a habit of drawing icons on buttons even when 
> this feature was turned off in the user's setting. This was mostly noticeable 
> in applications built on kdelibs4support.
> 
> It seems that the actual culprit is in Qt's QPushButton implementation 
> (https://bugreports.qt.io/browse/QTBUG-49887), but it is possible to work 
> around it in `KPushButton::paintEvent`, by removing the icon (forcing it to 
> the null icon) in the option instance, before handing off control to the 
> painter.
> 
> 
> Diffs
> -
> 
>   src/kdeui/kpushbutton.cpp 98534fa 
> 
> Diff: https://git.reviewboard.kde.org/r/126308/diff/
> 
> 
> Testing
> ---
> 
> On Kubuntu 14.04 and OS X 10.9.5 with Qt 5.5.1 and KF5 frameworks 5.16.0 .
> 
> I have not yet verified if there are other classes where this modification 
> would be relevant too.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126308: KPushButton: respect SH_DialogButtonBox_ButtonsHaveIcons, also when drawing

[Thomas Lübking]

> On Dec. 10, 2015, 10:11 p.m., Thomas Lübking wrote:
> > 1. What tells you that this is a dialog buttonbox pushbutton?
> > 2. What happens if the button has no text?
> > 
> > 
> > The bug is in QDialogButtonBox (or rather the K variant, 
> > QDialogButtonBoxPrivate::createButton() seems to incorporate the hint 
> > correctly)
> > 
> > 
> > [KDE] has "ShowIconsOnPushButtons=false" in kdeglobals which was 
> > interpreted by KPushButton _and_ kstyle for the hint, but the hint only 
> > covers Q'DialogButtonBox'es - there's simply no global rule for this like 
> > AA_DontShowIconsInMenus
> > 
> > => KDialogButtonBox shall respect the hint for its buttons (there're two 
> > special creation routines).
> > 
> > For the rest, the platform plugin ideally picks the kdeglobals setting and 
> > exports it to the application object (dyn property?) where the style can 
> > pick it and incorporate it into its calculations (ie. if no icons are 
> > wanted and there's text or arrow, omit the icon in size calculation and 
> > painting)
> > 
> > "Fixing" that in deprecated KPushButton doesn't really fix anything. We'll 
> > face the mix we had, just that users of QPushButton were far less prone to 
> > pass them an icon in pre-KF5 times.
> > 
> > Please also attach Hugo Pereira Da Costa.
> 
> René J.V. Bertin wrote:
> 1: why should one care? It is said nowhere that the setting defined as 
> "show icons on buttons" in `systemsettings(5)` applies only to dialogs. 
> Rather, the tooltip says "when this is enabled, KDE applications will show 
> icons alongside [sic!] some important buttons".
> In other words, when "this" is *not* enabled, there should be no icons, 
> period.
> I have found no sign in the code that the ShowIconsOnPushButtons hint is 
> to be used only for dialogs; `SH_DialogButtonBox_ButtonsHaveIcons` indeed 
> carries the suggestion in its name but I would not be surprised if Qt really 
> thinks of it in a more general sense. Probably also because the notion of 
> what a dialog is has become a lot vaguer.
> 
> And that also happens to be what Qt does; buttons show their icons on 
> Linux (and other Unix variants?) but on OS X or MS Windows displaying of 
> those icons is deactivated unless you use a style that enables it. In fact, 
> the default setting for `SH_DialogButtonBox_ButtonsHaveIcons` is false except 
> in the generic Unix theme (= it does work globally like 
> `AA_DontShowIconsInMenus` everywhere else).
> 
> 2: a user who indicates s/he doesn't want to see icons will get an empty 
> button. That's also what can happen with QPushButton, and app developers have 
> to take this into consideration. Cf. toolbars (and Qt's position on the use 
> of "texted separators").
> I don't think I've ever come across a standard button showing only an 
> icon, except possibly the arrow button next to the progress indicators in 
> KMail and KDevelop.
> 
> As to fixing it here: as it turns out, "here" is the main source for 
> annoying icons rearing their silly heads on buttons on my screen ;) and it 
> was also something of a challenge to understand why they kept appearing 
> despite the fact that all code appeared to return the value of 
> `ShowIconsOnPushButtons`. Deprecated or not, it doesn't look like all 
> applications are going to stop using it anytime soon.
> 
> I looked into fixing the issue in KDialogButtonBox but saw that it does 
> nothing to override the `ShowIconsOnPushButtons` setting. The only way to 
> respect the setting through that class (or a modern equivalent) would be to 
> set an empty icon if `ShowIconsOnPushButtons=false`. That introduces another 
> regression: changes in this setting are supposed to be reflected by running 
> applications without requiring a restart (or a recreation of dialogs). If it 
> were just me I'd decree that buttons can have either text or an icon, but 
> right now we have to make do with this mixed situation.
> 
> I don't mind making this an OS X (and MS Windows) specific modification, 
> of course, but on those platforms

> 1: why should one care?

Because, as explained, that is what the hint says. Nothing else.

> I have found no sign in the code that the ShowIconsOnPushButtons hint is to 
> be used only for dialogs

No, but it's been used to feed SH_DialogButtonBox_ButtonsHaveIcons *AND* 
KPushButton.
ShowIconsOnPushButtons implied SH_DialogButtonBox_ButtonsHaveIcons but NOT vv.

The approach is wrong, since you're abusing the hint for generalisation.

> but on OS X or MS Windows

... Qt uses native el

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


Errr... you intend to crash applications depending on whether some file is 
present??
Please fix the actual bug instead of such workaround, got a backtrace?

- Thomas Lübking


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.

I didn't say this patch would introduce a crash.
I said that "something" crashes for "some" reason and you work around that by 
testing whether a file is present.
Your own commit message clearly says that if - for any reason - 
~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
what means that the actual problem is not resolved but just shadowed.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
> I didn't say this patch would introduce a crash.
> I said that "something" crashes for "some" reason and you work around 
> that by testing whether a file is present.
> Your own commit message clearly says that if - for any reason - 
> ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
> what means that the actual problem is not resolved but just shadowed.
> 
> Boudhayan Gupta wrote:
> Why would you want to touch ~/.local/share/baloo/index, if you disable 
> baloo?
> 
> Let me explain - balooctl disable works by disabling indexing and 
> removing ~/.local/share/baloo/index. If baloo is disabled, that file is not 
> supposed to exist. A couple of months ago we were even trying to figure out 
> how that file came to exist on systems with Baloo disabled and now I've found 
> out. So this fixes that too.
> 
> Now the real problem. Before this patch, Baloo::Database::open() would 
> just create an empty database. Fair enough, except that db->open() would 
> return true in places where you clearly have an invalid database. This would 
> still work (i.e., not crash), because invalid in this context is empty. 
> However, once you select a ton of files (the number is very weird  - 158 or 
> something) LMDB would scream at you with this problem - **MDB_READERS_FULL: 
> Environment maxreaders limit reached**
> 
> There are two ways to solve this - increase the maxreaders limit in LMDB, 
> or return false if the database doesn't exist. The first option is when you 
> want to go all Linus - *we do not break userspace*, but this way is the more 
> correct way of solving this, because again, on systems where Baloo is 
> disabled, the index file isn't supposed to exist at all.
> 
> I was afraid I was opening up a can of worms by adding yet another 
> condition for db->open() to return false, because I was afraid there were 
> places where a check on db->open()'s return value was missing (and someone 
> would try to do operations on an invalid database). However, I've done all 
> the things that used to trigger crashes before and nothing's crashed yet. 
> With Baloo being both enabled and disabled.
> 
> Thomas Lübking wrote:
> That sounds as if a ton of jobs is started which all try to access the 
> database?
> W/o knowing the details on what is going on, there should probably:
> 
> a) a sane limit per client on how many DB jobs to perform at once 
> (general performance issue)
> b) a sanity check on the index file ie. testing whether it contains some 
> significant bytes or at least isn't empty.
> 
> I guess (b) should actually done by lmdb, but it won't hurt to do it on 
> baloo as well
> 
> Is this also a problem if the file is actually a valid DB, but baloo just 
> disabled?
> 
> 
> ---
> General remark: I guess valid databases should not be deleted but at best 
> be renamed with de/activation, no matter what else happens with this patch.
> 
> Boudhayan Gupta wrote:
> I'll put this down to a LMDB bug (I suspect some race condition) because 
> when the db is active and indexing is enabled, the crash doesn't happen. 
> Therefore I see no sense in increasing the limit. As for b), then I'd have to 
> know the LMDB file format to do it in Baloo (by default an empty database as 
> created by this method is 8KB in size, so there's some sort of data in it).
> 
> For your general remark (rename dbs instead of deleting them) - you'd 
> have to ask Vishesh. That's a design decision left to the maintainer. I'm 
> just a drive-by patcher who's somewhat familiar with the codebase because 
> I've fixed quite a few crashes in other apps when Baloo is disabled ;-)

> Therefore I see no sense in increasing the limit.

Not "increase", the opposite - but Vishesh already argued in the same direction 
;-)

It would seem that one of mdb_dbi_open, mdb_dbi_stat or mdb_dbi_flags should 
better return !0 if the database is invalid ...?


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#re

Re: Review Request 126105: Fix yet another crash in Dolphin when Baloo isn't running

[Thomas Lübking]

> On Nov. 18, 2015, 8:06 nachm., Thomas Lübking wrote:
> > Errr... you intend to crash applications depending on whether some file is 
> > present??
> > Please fix the actual bug instead of such workaround, got a backtrace?
> 
> Boudhayan Gupta wrote:
> Err... it **doesn't** cause a crash with the patch, causes a crash 
> without it. This is a perfectly valid fix - this function is **supposed** to 
> return false if the database could not be opened, for any reason whatsoever.
> 
> Thomas Lübking wrote:
> I didn't say this patch would introduce a crash.
> I said that "something" crashes for "some" reason and you work around 
> that by testing whether a file is present.
> Your own commit message clearly says that if - for any reason - 
> ~/.local/share/baloo/index exists (eg. i touch it), things will crash again, 
> what means that the actual problem is not resolved but just shadowed.
> 
> Boudhayan Gupta wrote:
> Why would you want to touch ~/.local/share/baloo/index, if you disable 
> baloo?
> 
> Let me explain - balooctl disable works by disabling indexing and 
> removing ~/.local/share/baloo/index. If baloo is disabled, that file is not 
> supposed to exist. A couple of months ago we were even trying to figure out 
> how that file came to exist on systems with Baloo disabled and now I've found 
> out. So this fixes that too.
> 
> Now the real problem. Before this patch, Baloo::Database::open() would 
> just create an empty database. Fair enough, except that db->open() would 
> return true in places where you clearly have an invalid database. This would 
> still work (i.e., not crash), because invalid in this context is empty. 
> However, once you select a ton of files (the number is very weird  - 158 or 
> something) LMDB would scream at you with this problem - **MDB_READERS_FULL: 
> Environment maxreaders limit reached**
> 
> There are two ways to solve this - increase the maxreaders limit in LMDB, 
> or return false if the database doesn't exist. The first option is when you 
> want to go all Linus - *we do not break userspace*, but this way is the more 
> correct way of solving this, because again, on systems where Baloo is 
> disabled, the index file isn't supposed to exist at all.
> 
> I was afraid I was opening up a can of worms by adding yet another 
> condition for db->open() to return false, because I was afraid there were 
> places where a check on db->open()'s return value was missing (and someone 
> would try to do operations on an invalid database). However, I've done all 
> the things that used to trigger crashes before and nothing's crashed yet. 
> With Baloo being both enabled and disabled.

That sounds as if a ton of jobs is started which all try to access the database?
W/o knowing the details on what is going on, there should probably:

a) a sane limit per client on how many DB jobs to perform at once (general 
performance issue)
b) a sanity check on the index file ie. testing whether it contains some 
significant bytes or at least isn't empty.

I guess (b) should actually done by lmdb, but it won't hurt to do it on baloo 
as well

Is this also a problem if the file is actually a valid DB, but baloo just 
disabled?


---
General remark: I guess valid databases should not be deleted but at best be 
renamed with de/activation, no matter what else happens with this patch.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126105/#review88544
---


On Nov. 18, 2015, 7:40 nachm., Boudhayan Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126105/
> ---
> 
> (Updated Nov. 18, 2015, 7:40 nachm.)
> 
> 
> Review request for Baloo, KDE Frameworks, Pinak Ahuja, and Vishesh Handa.
> 
> 
> Repository: baloo
> 
> 
> Description
> ---
> 
> Add a check in Baloo::Database::open() to return false if we're opening the 
> database in ReadOnly mode and the database doesn't exist. This fixes a crash 
> in Dolphin when Baloo isn't running.
> 
> This isn't the entire fix - one will also have to remove 
> ~/.local/share/baloo/index to not crash anymore; this patch prevents the file 
> from being recreated everytime Baloo::Database::open() is run, and re-causing 
> the crash.
> 
> 
> Diffs
> -
> 
>   src/engine/database.cpp 4f0579f 
> 
> Diff: https://git.reviewboard.kde.org/r/126105/diff/
> 
> 
> Testing
> ---
> 
> Builds, runs, doesn't crash anymore, the works.
> 
> 
> Thanks,
> 
> Boudhayan Gupta
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125658: Do not treat the context menu button as modifier

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125658/
---

(Updated Oct. 19, 2015, 7:21 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Christoph Feck and Hans Chen.


Changes
---

Submitted with commit 3b690ae01b31faaddaa38ca6ef5425ef68f7546c by Thomas 
Lübking to branch master.


Bugs: 165542
https://bugs.kde.org/show_bug.cgi?id=165542


Repository: kxmlgui


Description
---

It's not and not treated as such by Qt either.

In addition suck context events while recording
(so it doesn't activate, rather cosmetic)

BUG: 165542
FIXED-IN: 5.16
CHANGELOG: Allow to bind the contextmenu key (lower right) to shortcuts


Diffs
-

  src/kkeysequencewidget.cpp 2385f20 

Diff: https://git.reviewboard.kde.org/r/125658/diff/


Testing
---

yupp.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125658: Do not treat the context menu button as modifier

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125658/
---

(Updated Okt. 18, 2015, 2:45 nachm.)


Review request for KDE Frameworks, Christoph Feck and Hans Chen.


Changes
---

getting a bit more audience for comments ;-)


Bugs: 165542
https://bugs.kde.org/show_bug.cgi?id=165542


Repository: kxmlgui


Description
---

It's not and not treated as such by Qt either.

In addition suck context events while recording
(so it doesn't activate, rather cosmetic)

BUG: 165542
FIXED-IN: 5.16
CHANGELOG: Allow to bind the contextmenu key (lower right) to shortcuts


Diffs
-

  src/kkeysequencewidget.cpp 2385f20 

Diff: https://git.reviewboard.kde.org/r/125658/diff/


Testing
---

yupp.


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125309: Support multiple X servers in the NETWM classes

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125309/
---

(Updated Sept. 29, 2015, 9:21 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Martin Gräßlin.


Changes
---

Submitted with commit 6404dde43127dbcecbc370ad57ae3523ca3c3325 by Thomas 
Lübking to branch master.


Repository: kwindowsystem


Description
---

alteration review #125259 minus the autotests

Significant difference is the creation and handling of the Atom enum.

1. Net::Utf8 makes no sense
2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE 
(there's nothing such as XA_WM_STATE)
3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, KDE_NET_, 
or ... UTF8, I'm not sure why one would add more enums (since this is internal 
API and the resolved type will always be xcb_atom_t - in worst case we run into 
errors on comparing the enums
4. I wanted to get rid of the explicit counter variable as well as any loose 
assignment between atom variable/enum and string.

-

So far on first creation of a NETRootInfo or NETWinInfo a static
initialization of atoms was performed. This meant that the NET classes
are only able to interact with the X server the first NET class is
connected to.

Normally applications don't need to interact with multiple X servers.
An exception is kwin_wayland which needs a connection to its Xwayland
server and on the x11 backend to the X server it renders to. So far
KWin could not use the NET classes for it, causing the rendering window
to e.g. not have a window title.

This change removes the implicit constraint on one X server by
creating a hash of connection and atoms. For each created NET class
we check whether we have already resolved the atoms, if yes we reuse
otherwise we create them.

For the normal use case of one X11 connection the change should be
rather minimal. Instead of performing the check whether the static
atoms have been created, it now is a check whether the atoms for the
connection have been created. The atoms are kept in a
QSharedDataPointer ensuring that we don't needless copy the atoms into
each class.

CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.


Diffs
-

  src/platforms/xcb/atoms_p.h PRE-CREATION 
  src/platforms/xcb/netwm.cpp d99a925 
  src/platforms/xcb/netwm_p.h 917a86e 

Diff: https://git.reviewboard.kde.org/r/125309/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125309: Support multiple X servers in the NETWM classes

[Thomas Lübking]

> On Sept. 28, 2015, 12:56 nachm., Martin Gräßlin wrote:
> > src/platforms/xcb/netwm_p.h, line 39
> > <https://git.reviewboard.kde.org/r/125309/diff/1/?file=404731#file404731line39>
> >
> > I don't like the type name "Atom" as that's also an XLib type. As the 
> > actual name is hardly used we could get it to something more precise, maybe 
> > NetAtom?
> 
> Thomas Lübking wrote:
> "KwsAtom"?
> 
> Martin Gräßlin wrote:
> sounds good, better description than NetAtom

(it's not always a _NET_WM atom)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125309/#review86036
-----------


On Sept. 19, 2015, 1:22 nachm., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125309/
> ---
> 
> (Updated Sept. 19, 2015, 1:22 nachm.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> alteration review #125259 minus the autotests
> 
> Significant difference is the creation and handling of the Atom enum.
> 
> 1. Net::Utf8 makes no sense
> 2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE 
> (there's nothing such as XA_WM_STATE)
> 3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, 
> KDE_NET_, or ... UTF8, I'm not sure why one would add more enums (since this 
> is internal API and the resolved type will always be xcb_atom_t - in worst 
> case we run into errors on comparing the enums
> 4. I wanted to get rid of the explicit counter variable as well as any loose 
> assignment between atom variable/enum and string.
> 
> -
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/netwm_p.h 917a86e 
>   src/platforms/xcb/atoms_p.h PRE-CREATION 
>   src/platforms/xcb/netwm.cpp d99a925 
> 
> Diff: https://git.reviewboard.kde.org/r/125309/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125309: Support multiple X servers in the NETWM classes

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125309/
---

(Updated Sept. 28, 2015, 3:45 nachm.)


Review request for KDE Frameworks and Martin Gräßlin.


Changes
---

* Atom -> KwsAtom
* undefine ENUM_CREATE_CHAR_ARRAY
* explain macro jungle and howto include
* all our atoms now belong to me >-)


Repository: kwindowsystem


Description
---

alteration review #125259 minus the autotests

Significant difference is the creation and handling of the Atom enum.

1. Net::Utf8 makes no sense
2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE 
(there's nothing such as XA_WM_STATE)
3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, KDE_NET_, 
or ... UTF8, I'm not sure why one would add more enums (since this is internal 
API and the resolved type will always be xcb_atom_t - in worst case we run into 
errors on comparing the enums
4. I wanted to get rid of the explicit counter variable as well as any loose 
assignment between atom variable/enum and string.

-

So far on first creation of a NETRootInfo or NETWinInfo a static
initialization of atoms was performed. This meant that the NET classes
are only able to interact with the X server the first NET class is
connected to.

Normally applications don't need to interact with multiple X servers.
An exception is kwin_wayland which needs a connection to its Xwayland
server and on the x11 backend to the X server it renders to. So far
KWin could not use the NET classes for it, causing the rendering window
to e.g. not have a window title.

This change removes the implicit constraint on one X server by
creating a hash of connection and atoms. For each created NET class
we check whether we have already resolved the atoms, if yes we reuse
otherwise we create them.

For the normal use case of one X11 connection the change should be
rather minimal. Instead of performing the check whether the static
atoms have been created, it now is a check whether the atoms for the
connection have been created. The atoms are kept in a
QSharedDataPointer ensuring that we don't needless copy the atoms into
each class.

CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.


Diffs (updated)
-

  src/platforms/xcb/atoms_p.h PRE-CREATION 
  src/platforms/xcb/netwm.cpp d99a925 
  src/platforms/xcb/netwm_p.h 917a86e 

Diff: https://git.reviewboard.kde.org/r/125309/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125259: Support multiple X servers in the NETWM classes

[Thomas Lübking]

> On Sept. 18, 2015, 12:21 nachm., Thomas Lübking wrote:
> > src/platforms/xcb/netwm_p.h, line 31
> > <https://git.reviewboard.kde.org/r/125259/diff/2/?file=404639#file404639line31>
> >
> > errrhemmm ... the idea was actually to keep things more in sync and not 
> > have to match variable, string assignment and netAtomCount by hand, so I 
> > would have actually gone for including an atom header twice on different 
> > macro definition.
> > 
> > Also the presented system drops scope (KDE_NET_WM, NET_WM, WM and ... 
> > UTF8_STRING ;-)
> > 
> > Sorry :-(
> 
> Martin Gräßlin wrote:
> sorry, now you have lost me with what you actually want.

Gonna write a patch. Promise masses of red ;-)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125259/#review85619
---


On Sept. 18, 2015, 8:19 vorm., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125259/
> ---
> 
> (Updated Sept. 18, 2015, 8:19 vorm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> [autotests] NETWM tests get a new X server per test
> 
> Making use of new feature of allowing multiple X connections:
> start X server in init() instead of initTestCase().
> 
> 
> Diffs
> -
> 
>   autotests/netwininfotestwm.cpp a98e12fd690b0250337c7588e1525af1d0dda38c 
>   autotests/netrootinfotestwm.cpp e918873a5281f6ecbcc1769de3dadcf8f6222f6a 
>   autotests/netwininfotestclient.cpp a5b10376b943ea914a9521b5c07f9ef13a65d2f1 
>   src/platforms/xcb/netwm.cpp d99a925ad2b99d5e60ab1dafcb01400b4a6a4c93 
>   src/platforms/xcb/netwm_p.h 917a86ed5b6c83f36e73bbc346360b943d457243 
> 
> Diff: https://git.reviewboard.kde.org/r/125259/diff/
> 
> 
> Testing
> ---
> 
> see adjusted unit tests. Also tried it in kwin_wayland
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 125309: Support multiple X servers in the NETWM classes

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125309/
---

Review request for KDE Frameworks and Martin Gräßlin.


Repository: kwindowsystem


Description
---

alteration review #125259 minus the autotests

Significant difference is the creation and handling of the Atom enum.

1. Net::Utf8 makes no sense
2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE 
(there's nothing such as XA_WM_STATE)
3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, KDE_NET_, 
or ... UTF8, I'm not sure why one would add more enums (since this is internal 
API and the resolved type will always be xcb_atom_t - in worst case we run into 
errors on comparing the enums
4. I wanted to get rid of the explicit counter variable as well as any loose 
assignment between atom variable/enum and string.

-

So far on first creation of a NETRootInfo or NETWinInfo a static
initialization of atoms was performed. This meant that the NET classes
are only able to interact with the X server the first NET class is
connected to.

Normally applications don't need to interact with multiple X servers.
An exception is kwin_wayland which needs a connection to its Xwayland
server and on the x11 backend to the X server it renders to. So far
KWin could not use the NET classes for it, causing the rendering window
to e.g. not have a window title.

This change removes the implicit constraint on one X server by
creating a hash of connection and atoms. For each created NET class
we check whether we have already resolved the atoms, if yes we reuse
otherwise we create them.

For the normal use case of one X11 connection the change should be
rather minimal. Instead of performing the check whether the static
atoms have been created, it now is a check whether the atoms for the
connection have been created. The atoms are kept in a
QSharedDataPointer ensuring that we don't needless copy the atoms into
each class.

CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.


Diffs
-

  src/platforms/xcb/netwm_p.h 917a86e 
  src/platforms/xcb/atoms_p.h PRE-CREATION 
  src/platforms/xcb/netwm.cpp d99a925 

Diff: https://git.reviewboard.kde.org/r/125309/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125309: Support multiple X servers in the NETWM classes

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125309/#review85666
---



src/platforms/xcb/atoms_p.h (line 25)
<https://git.reviewboard.kde.org/r/125309/#comment59186>

I didn't really re-visit the grouping and comments in the original array 
contructor; We might want to change order here a bit (but that's not really 
important =)


- Thomas Lübking


On Sept. 19, 2015, 1:22 nachm., Thomas Lübking wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125309/
> ---
> 
> (Updated Sept. 19, 2015, 1:22 nachm.)
> 
> 
> Review request for KDE Frameworks and Martin Gräßlin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> alteration review #125259 minus the autotests
> 
> Significant difference is the creation and handling of the Atom enum.
> 
> 1. Net::Utf8 makes no sense
> 2. Net::WmFoo re/adds ambiguity between _NET_WM and WM_, notably on _STATE 
> (there's nothing such as XA_WM_STATE)
> 3. the most "important" grouping is whether it's WM_, NET_WM_, NET_, 
> KDE_NET_, or ... UTF8, I'm not sure why one would add more enums (since this 
> is internal API and the resolved type will always be xcb_atom_t - in worst 
> case we run into errors on comparing the enums
> 4. I wanted to get rid of the explicit counter variable as well as any loose 
> assignment between atom variable/enum and string.
> 
> -
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> 
> Diffs
> -
> 
>   src/platforms/xcb/netwm_p.h 917a86e 
>   src/platforms/xcb/atoms_p.h PRE-CREATION 
>   src/platforms/xcb/netwm.cpp d99a925 
> 
> Diff: https://git.reviewboard.kde.org/r/125309/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125259: Support multiple X servers in the NETWM classes

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125259/#review85619
---



src/platforms/xcb/netwm_p.h (line 31)
<https://git.reviewboard.kde.org/r/125259/#comment59166>

errrhemmm ... the idea was actually to keep things more in sync and not 
have to match variable, string assignment and netAtomCount by hand, so I would 
have actually gone for including an atom header twice on different macro 
definition.

Also the presented system drops scope (KDE_NET_WM, NET_WM, WM and ... 
UTF8_STRING ;-)

Sorry :-(


- Thomas Lübking


On Sept. 18, 2015, 8:19 vorm., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125259/
> ---
> 
> (Updated Sept. 18, 2015, 8:19 vorm.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> [autotests] NETWM tests get a new X server per test
> 
> Making use of new feature of allowing multiple X connections:
> start X server in init() instead of initTestCase().
> 
> 
> Diffs
> -
> 
>   autotests/netwininfotestwm.cpp a98e12fd690b0250337c7588e1525af1d0dda38c 
>   autotests/netrootinfotestwm.cpp e918873a5281f6ecbcc1769de3dadcf8f6222f6a 
>   autotests/netwininfotestclient.cpp a5b10376b943ea914a9521b5c07f9ef13a65d2f1 
>   src/platforms/xcb/netwm.cpp d99a925ad2b99d5e60ab1dafcb01400b4a6a4c93 
>   src/platforms/xcb/netwm_p.h 917a86ed5b6c83f36e73bbc346360b943d457243 
> 
> Diff: https://git.reviewboard.kde.org/r/125259/diff/
> 
> 
> Testing
> ---
> 
> see adjusted unit tests. Also tried it in kwin_wayland
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125259: Support multiple X servers in the NETWM classes

[Thomas Lübking]

> On Sept. 16, 2015, 4:23 nachm., Thomas Lübking wrote:
> > would one want to use the occasion to get rid of the bazillion variables 
> > mapped to a struct array and have atom(Atom a) and atomString(Atom a) for 
> > an array of atoms a matching string - maybe including some namespaces so 
> > that instead of p->atom->net_wm_to_many_underscores one would have 
> > atom(NET::WM::NoUnderscores) and atom(WM::NoUnderscores) etc.?
> > 
> > Ideally with a little preprocessor and include magic?
> > http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c
> 
> Martin Gräßlin wrote:
> rather orthogonal to the change. I'm not sure whether a huge switch 
> statemement will make it better. What might be more interesting is to get 
> something like KWin's Xcb::Atom in to also only fetch the atoms which are 
> actually used. Though the Atom class is of course considerable larger than 
> just an xcb_atom_t.

The switch in the demo code is just a usage demo - I'd promise red overweight 
;-)
Constantly using atom(Atom a) would of course allow a sparse array that mostly 
contains of "0" and is only filled on demand.

The reason why I thought it might be a good idea *now* is because this already 
requires a HUGE change that will block git blame.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125259/#review85501
---


On Sept. 16, 2015, 1:42 nachm., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125259/
> ---
> 
> (Updated Sept. 16, 2015, 1:42 nachm.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> [autotests] NETWM tests get a new X server per test
> 
> Making use of new feature of allowing multiple X connections:
> start X server in init() instead of initTestCase().
> 
> 
> Diffs
> -
> 
>   autotests/netrootinfotestwm.cpp e918873a5281f6ecbcc1769de3dadcf8f6222f6a 
>   autotests/netwininfotestclient.cpp a5b10376b943ea914a9521b5c07f9ef13a65d2f1 
>   autotests/netwininfotestwm.cpp a98e12fd690b0250337c7588e1525af1d0dda38c 
>   src/platforms/xcb/netwm.cpp d99a925ad2b99d5e60ab1dafcb01400b4a6a4c93 
>   src/platforms/xcb/netwm_p.h 917a86ed5b6c83f36e73bbc346360b943d457243 
> 
> Diff: https://git.reviewboard.kde.org/r/125259/diff/
> 
> 
> Testing
> ---
> 
> see adjusted unit tests. Also tried it in kwin_wayland
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125259: Support multiple X servers in the NETWM classes

[Thomas Lübking]

> On Sept. 16, 2015, 4:23 nachm., Thomas Lübking wrote:
> > would one want to use the occasion to get rid of the bazillion variables 
> > mapped to a struct array and have atom(Atom a) and atomString(Atom a) for 
> > an array of atoms a matching string - maybe including some namespaces so 
> > that instead of p->atom->net_wm_to_many_underscores one would have 
> > atom(NET::WM::NoUnderscores) and atom(WM::NoUnderscores) etc.?
> > 
> > Ideally with a little preprocessor and include magic?
> > http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c
> 
> Martin Gräßlin wrote:
> rather orthogonal to the change. I'm not sure whether a huge switch 
> statemement will make it better. What might be more interesting is to get 
> something like KWin's Xcb::Atom in to also only fetch the atoms which are 
> actually used. Though the Atom class is of course considerable larger than 
> just an xcb_atom_t.
> 
> Thomas Lübking wrote:
> The switch in the demo code is just a usage demo - I'd promise red 
> overweight ;-)
> Constantly using atom(Atom a) would of course allow a sparse array that 
> mostly contains of "0" and is only filled on demand.
> 
> The reason why I thought it might be a good idea *now* is because this 
> already requires a HUGE change that will block git blame.
> 
> Martin Gräßlin wrote:
> > Constantly using atom(Atom a) would of course allow a sparse array that 
> mostly contains of "0" and is only filled on demand.
> 
> That sounds of having other disadvantages: round trips as we would need 
> to resolve the atom when needed. Also the SharedDataPointer in the patch 
> would not work as it would detach them on fetch and cause multiple NETWinInfo 
> to catch the atoms again.
> 
> So overall from the minimize roundtrip perspective fetching all atoms on 
> first run still sounds like the best idea...

"to get something like KWin's Xcb::Atom in to also only fetch the atoms which 
are actually used." ... :-P

> SharedDataPointer in the patch would not work as it would detach them on fetch

For "Atom" i actually meant an enum, not KWin's XCB::Atom - I don't see where 
this would detach anything, but the roundtrips would oc. occur.
One could decide to load a "standard" set on init and the rest only on demand, 
but you don't win memory by this (just "maybe" a little time on first NETWM 
invocation)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125259/#review85501
---


On Sept. 16, 2015, 1:42 nachm., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125259/
> ---
> 
> (Updated Sept. 16, 2015, 1:42 nachm.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> [autotests] NETWM tests get a new X server per test
> 
> Making use of new feature of allowing multiple X connections:
> start X server in init() instead of initTestCase().
> 
> 
> Diffs
> -
> 
>   autotests/netrootinfotestwm.cpp e918873a5281f6ecbcc1769de3dadcf8f6222f

Re: Review Request 125259: Support multiple X servers in the NETWM classes

[Thomas Lübking]

> On Sept. 16, 2015, 4:23 nachm., Thomas Lübking wrote:
> > would one want to use the occasion to get rid of the bazillion variables 
> > mapped to a struct array and have atom(Atom a) and atomString(Atom a) for 
> > an array of atoms a matching string - maybe including some namespaces so 
> > that instead of p->atom->net_wm_to_many_underscores one would have 
> > atom(NET::WM::NoUnderscores) and atom(WM::NoUnderscores) etc.?
> > 
> > Ideally with a little preprocessor and include magic?
> > http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c
> 
> Martin Gräßlin wrote:
> rather orthogonal to the change. I'm not sure whether a huge switch 
> statemement will make it better. What might be more interesting is to get 
> something like KWin's Xcb::Atom in to also only fetch the atoms which are 
> actually used. Though the Atom class is of course considerable larger than 
> just an xcb_atom_t.
> 
> Thomas Lübking wrote:
> The switch in the demo code is just a usage demo - I'd promise red 
> overweight ;-)
> Constantly using atom(Atom a) would of course allow a sparse array that 
> mostly contains of "0" and is only filled on demand.
> 
> The reason why I thought it might be a good idea *now* is because this 
> already requires a HUGE change that will block git blame.
> 
> Martin Gräßlin wrote:
> > Constantly using atom(Atom a) would of course allow a sparse array that 
> mostly contains of "0" and is only filled on demand.
> 
> That sounds of having other disadvantages: round trips as we would need 
> to resolve the atom when needed. Also the SharedDataPointer in the patch 
> would not work as it would detach them on fetch and cause multiple NETWinInfo 
> to catch the atoms again.
> 
> So overall from the minimize roundtrip perspective fetching all atoms on 
> first run still sounds like the best idea...
> 
> Thomas Lübking wrote:
> "to get something like KWin's Xcb::Atom in to also only fetch the atoms 
> which are actually used." ... :-P
> 
> > SharedDataPointer in the patch would not work as it would detach them 
> on fetch
> 
> For "Atom" i actually meant an enum, not KWin's XCB::Atom - I don't see 
> where this would detach anything, but the roundtrips would oc. occur.
> One could decide to load a "standard" set on init and the rest only on 
> demand, but you don't win memory by this (just "maybe" a little time on first 
> NETWM invocation)
> 
> Martin Gräßlin wrote:
> > "to get something like KWin's Xcb::Atom in to also only fetch the atoms 
> which are actually used." ... :-P
> 
> KWin's Xcb::Atom doesn't cause roundtrips.
> 
> > I don't see where this would detach anything
> 
> From QSharedDataPointer documentation "For write access operator ->() 
> will automatically call detach()". My understanding is that if we update the 
> value of an xcb_atom_t then it would detach. Granted one could change to 
> QExplicitlySharedDataPointer.
> 
> David Faure wrote:
> QSharedDataPointer is used to write value classes, where const methods do 
> not detach, and non-const methods do detach.
> 
> It sounds like you wanted to use QSharedPointer instead (refcounting).
> 
> Martin Gräßlin wrote:
> > It sounds like you wanted to use QSharedPointer instead
> 
> No, the class I added only has const methods.

Yes, but whenever or however you want to delay atom creation, that's done.

It would work for XCB::Atom only because "xcb_atom_t() const" performs a const 
cast on ::getReply(), so one would cheat "atom(AtomEnum a) const" in exactly 
the same way - and likely require a mutex in either case.

Afaiu, what you'd prefer in the end was an XCB::Atom-a-like approach, but w/ 
external m_connection pointer and a

union {
xcb_intern_atom_cookie_t cookie;
xcb_atom_t atom;
} u;

?

Apple has that [1] (but) it (only) works as the cookie struct consist of only 
one unsigned integer.

[1] 
http://www.opensource.apple.com/source/X11libs/X11libs-40/xcb-util/xcb-util-0.3.3/atom/xcb_atom.h


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125259/#review85501
---


On Sept. 16, 2015, 1:42 nachm., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/12525

Re: Review Request 125259: Support multiple X servers in the NETWM classes

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125259/#review85501
---


would one want to use the occasion to get rid of the bazillion variables mapped 
to a struct array and have atom(Atom a) and atomString(Atom a) for an array of 
atoms a matching string - maybe including some namespaces so that instead of 
p->atom->net_wm_to_many_underscores one would have atom(NET::WM::NoUnderscores) 
and atom(WM::NoUnderscores) etc.?

Ideally with a little preprocessor and include magic?
http://stackoverflow.com/questions/147267/easy-way-to-use-variables-of-enum-types-as-string-in-c

- Thomas Lübking


On Sept. 16, 2015, 1:42 nachm., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125259/
> ---
> 
> (Updated Sept. 16, 2015, 1:42 nachm.)
> 
> 
> Review request for KDE Frameworks and kwin.
> 
> 
> Repository: kwindowsystem
> 
> 
> Description
> ---
> 
> So far on first creation of a NETRootInfo or NETWinInfo a static
> initialization of atoms was performed. This meant that the NET classes
> are only able to interact with the X server the first NET class is
> connected to.
> 
> Normally applications don't need to interact with multiple X servers.
> An exception is kwin_wayland which needs a connection to its Xwayland
> server and on the x11 backend to the X server it renders to. So far
> KWin could not use the NET classes for it, causing the rendering window
> to e.g. not have a window title.
> 
> This change removes the implicit constraint on one X server by
> creating a hash of connection and atoms. For each created NET class
> we check whether we have already resolved the atoms, if yes we reuse
> otherwise we create them.
> 
> For the normal use case of one X11 connection the change should be
> rather minimal. Instead of performing the check whether the static
> atoms have been created, it now is a check whether the atoms for the
> connection have been created. The atoms are kept in a
> QSharedDataPointer ensuring that we don't needless copy the atoms into
> each class.
> 
> CHANGELOG: Allow interacting with multiple X servers in the NETWM classes.
> 
> [autotests] NETWM tests get a new X server per test
> 
> Making use of new feature of allowing multiple X connections:
> start X server in init() instead of initTestCase().
> 
> 
> Diffs
> -
> 
>   autotests/netrootinfotestwm.cpp e918873a5281f6ecbcc1769de3dadcf8f6222f6a 
>   autotests/netwininfotestclient.cpp a5b10376b943ea914a9521b5c07f9ef13a65d2f1 
>   autotests/netwininfotestwm.cpp a98e12fd690b0250337c7588e1525af1d0dda38c 
>   src/platforms/xcb/netwm.cpp d99a925ad2b99d5e60ab1dafcb01400b4a6a4c93 
>   src/platforms/xcb/netwm_p.h 917a86ed5b6c83f36e73bbc346360b943d457243 
> 
> Diff: https://git.reviewboard.kde.org/r/125259/diff/
> 
> 
> Testing
> ---
> 
> see adjusted unit tests. Also tried it in kwin_wayland
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123376: Fix handling of mod-shift-number shortcuts.

[Thomas Lübking]

 On Aug. 12, 2015, 10:17 vorm., Thomas Lübking wrote:
  bump, we should get this fixed for 5.4
  
  Shui, do you want to work on improving the patch and adding the testcase?
 
 Thomas Lübking wrote:
 -100
 THE PATCH BREAKS BACKTABBING!!
 
 Btw. entirely not figured by autotests :-P
 
 @Martin
 test might be quite a problem since this
 ```diff
 diff --git a/autotests/kglobalshortcuttest.cpp 
 b/autotests/kglobalshortcuttest.cpp
 index 30c932c..92c3f86 100644
 --- a/autotests/kglobalshortcuttest.cpp
 +++ b/autotests/kglobalshortcuttest.cpp
 @@ -37,6 +37,7 @@
  #include QX11Info
  #define XK_MISCELLANY
  #define XK_XKB_KEYS
 +#define XK_LATIN1
  #include X11/keysymdef.h
  #include xcb/xcb_keysyms.h
  #include xcb/xtest.h
 @@ -51,7 +52,7 @@
   *
  */
  
 -const QKeySequence sequenceA = QKeySequence(Qt::SHIFT + Qt::META + 
 Qt::CTRL + Qt::ALT + Qt::Key_F12);
 +const QKeySequence sequenceA = QKeySequence(Qt::SHIFT + Qt::META + 
 Qt::CTRL + Qt::ALT + Qt::Key_0/*Qt::Key_F12*/);
  const QKeySequence sequenceB = QKeySequence(Qt::Key_F29);
  const QKeySequence sequenceC = QKeySequence(Qt::SHIFT + Qt::META + 
 Qt::CTRL + Qt::Key_F28);
  const QKeySequence sequenceD = QKeySequence(Qt::META + Qt::ALT + 
 Qt::Key_F30);
 @@ -175,15 +176,16 @@ void KGlobalShortcutTest::testActivateShortcut()
  const xcb_keycode_t control = getCode(XK_Control_L);
  const xcb_keycode_t alt = getCode(XK_Alt_L);
  const xcb_keycode_t f12 = getCode(XK_F12);
 +const xcb_keycode_t _0 = getCode(XK_0);
  xcb_key_symbols_free(syms);
  
  xcb_test_fake_input(c, XCB_KEY_PRESS, meta,
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_PRESS, control, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_PRESS, alt, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_PRESS, shift,   
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 -xcb_test_fake_input(c, XCB_KEY_PRESS, f12, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 +xcb_test_fake_input(c, XCB_KEY_PRESS, _0, XCB_TIME_CURRENT_TIME, 
 w, 0, 0, 0);
  
 -xcb_test_fake_input(c, XCB_KEY_RELEASE, f12, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 +xcb_test_fake_input(c, XCB_KEY_RELEASE, _0, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_RELEASE, shift,   
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_RELEASE, meta,
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_RELEASE, control, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 @@ -191,6 +193,7 @@ void KGlobalShortcutTest::testActivateShortcut()
  xcb_flush(c);
  
  QVERIFY(actionASpy.wait());
 +
  QCOMPARE(actionASpy.count(), 1);
  #else
  QSKIP(This test requires to be compiled with XCB-XTEST);
 diff --git a/src/runtime/plugins/xcb/kglobalaccel_x11.cpp b/src/
 ```
 
 simple modification already fails on the trigger test, despite the 
 shortcut allocation on the live system works.
 The problem is that the registered key sequence is actually Qt::META + 
 Qt::CTRL + Qt::ALT + Qt::Key_Equal, and the bigger problem is, that this is 
 because of a german keyboard layout ...
 
 I assume we must fake in a key press and read the generated event (iow, 
 clone the shortcut editor behavior) for this.
 
 However: moot point. This patch breaks backtabbing, so can NO WAY GO IN!
 
 Thomas Lübking wrote:
 Weirdness may be in KKEyServer::isShiftAsModifierAllowed() which allows 
 Backtab, but not Tab.
 
 
 kkeysequencewidget.cpp however has special handling (which kglobaccel 
 would have to adapt, but this is a total mess)
 ```
if ((keyQt == Qt::Key_Backtab)  (d-modifierKeys  
 Qt::SHIFT)) {
 0758 keyQt = Qt::Key_Tab | d-modifierKeys;
 0759 } else if (KKeyServer::isShiftAsModifierAllowed(keyQt)) {
 0760 keyQt |= d-modifierKeys;
 ```
 
 Yuxuan Shui wrote:
 Any suggestions how to fix this?
 
 Thomas Lübking wrote:
 basically by ignoring Qt::Key_Tab | Qt::SHIFT and map Qt::Key_Backtab 
 to it (at least this should be what is grabbed)

See https://git.reviewboard.kde.org/r/124893/


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123376/#review83734
---


On April 25, 2015, 7:54 nachm., Yuxuan Shui wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123376/
 ---
 
 (Updated April 25

Re: Review Request 123376: Fix handling of mod-shift-number shortcuts.

[Thomas Lübking]

 On Mai 1, 2015, 7:51 vorm., Thomas Lübking wrote:
  Two things.
  a) 1 is a magic number (stupid xcb, should be xcppb ;-) - please add a 
  comment that this picks the 1st level shift
  b) the overall code looks a bit inefficient. No idea how smart xcb is under 
  the hood, but
 1. we fetch the keysym via xcb_key_press_lookup_keysym
 2. we check the mod state to be numlock
 3. we fetch the keysym again (xcb_key_press_lookup_keysym just wraps 
  xcb_key_symbols_get_keysym on ev-detail)
 4. we map keysymX to keysymQt
 5. we check the (mapped to Qt) modifier for the SHIFT flag
 6. this patch fetches and maps keysymX once more
   do you want to do a general cleanup with this or another patch?
 
 Yuxuan Shui wrote:
 Hi, sorry for the later reply. I've been really busy later.
 
 I can do a code cleanup (will have some free time 2 weeks from now).  
 What do you want me to do.

I'm  not entirely sure which applies here (different for runtime and lib?) but 
next releases are due on
Thu 2015-08-20 and Sat 2015-09-5

I'd otherwise fix this up (wrt autotests is possible) tomorrow otherwise.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123376/#review79753
---


On April 25, 2015, 7:54 nachm., Yuxuan Shui wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123376/
 ---
 
 (Updated April 25, 2015, 7:54 nachm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Bugs: 341959
 https://bugs.kde.org/show_bug.cgi?id=341959
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 For details, see discussion in corresponding bug report.
 
 
 Diffs
 -
 
   src/runtime/kglobalaccel_x11.cpp abee5bc 
 
 Diff: https://git.reviewboard.kde.org/r/123376/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Yuxuan Shui
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123376: Fix handling of mod-shift-number shortcuts.

[Thomas Lübking]

 On Aug. 12, 2015, 10:17 vorm., Thomas Lübking wrote:
  bump, we should get this fixed for 5.4
  
  Shui, do you want to work on improving the patch and adding the testcase?
 
 Thomas Lübking wrote:
 -100
 THE PATCH BREAKS BACKTABBING!!
 
 Btw. entirely not figured by autotests :-P
 
 @Martin
 test might be quite a problem since this
 ```diff
 diff --git a/autotests/kglobalshortcuttest.cpp 
 b/autotests/kglobalshortcuttest.cpp
 index 30c932c..92c3f86 100644
 --- a/autotests/kglobalshortcuttest.cpp
 +++ b/autotests/kglobalshortcuttest.cpp
 @@ -37,6 +37,7 @@
  #include QX11Info
  #define XK_MISCELLANY
  #define XK_XKB_KEYS
 +#define XK_LATIN1
  #include X11/keysymdef.h
  #include xcb/xcb_keysyms.h
  #include xcb/xtest.h
 @@ -51,7 +52,7 @@
   *
  */
  
 -const QKeySequence sequenceA = QKeySequence(Qt::SHIFT + Qt::META + 
 Qt::CTRL + Qt::ALT + Qt::Key_F12);
 +const QKeySequence sequenceA = QKeySequence(Qt::SHIFT + Qt::META + 
 Qt::CTRL + Qt::ALT + Qt::Key_0/*Qt::Key_F12*/);
  const QKeySequence sequenceB = QKeySequence(Qt::Key_F29);
  const QKeySequence sequenceC = QKeySequence(Qt::SHIFT + Qt::META + 
 Qt::CTRL + Qt::Key_F28);
  const QKeySequence sequenceD = QKeySequence(Qt::META + Qt::ALT + 
 Qt::Key_F30);
 @@ -175,15 +176,16 @@ void KGlobalShortcutTest::testActivateShortcut()
  const xcb_keycode_t control = getCode(XK_Control_L);
  const xcb_keycode_t alt = getCode(XK_Alt_L);
  const xcb_keycode_t f12 = getCode(XK_F12);
 +const xcb_keycode_t _0 = getCode(XK_0);
  xcb_key_symbols_free(syms);
  
  xcb_test_fake_input(c, XCB_KEY_PRESS, meta,
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_PRESS, control, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_PRESS, alt, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_PRESS, shift,   
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 -xcb_test_fake_input(c, XCB_KEY_PRESS, f12, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 +xcb_test_fake_input(c, XCB_KEY_PRESS, _0, XCB_TIME_CURRENT_TIME, 
 w, 0, 0, 0);
  
 -xcb_test_fake_input(c, XCB_KEY_RELEASE, f12, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 +xcb_test_fake_input(c, XCB_KEY_RELEASE, _0, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_RELEASE, shift,   
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_RELEASE, meta,
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
  xcb_test_fake_input(c, XCB_KEY_RELEASE, control, 
 XCB_TIME_CURRENT_TIME, w, 0, 0, 0);
 @@ -191,6 +193,7 @@ void KGlobalShortcutTest::testActivateShortcut()
  xcb_flush(c);
  
  QVERIFY(actionASpy.wait());
 +
  QCOMPARE(actionASpy.count(), 1);
  #else
  QSKIP(This test requires to be compiled with XCB-XTEST);
 diff --git a/src/runtime/plugins/xcb/kglobalaccel_x11.cpp b/src/
 ```
 
 simple modification already fails on the trigger test, despite the 
 shortcut allocation on the live system works.
 The problem is that the registered key sequence is actually Qt::META + 
 Qt::CTRL + Qt::ALT + Qt::Key_Equal, and the bigger problem is, that this is 
 because of a german keyboard layout ...
 
 I assume we must fake in a key press and read the generated event (iow, 
 clone the shortcut editor behavior) for this.
 
 However: moot point. This patch breaks backtabbing, so can NO WAY GO IN!
 
 Thomas Lübking wrote:
 Weirdness may be in KKEyServer::isShiftAsModifierAllowed() which allows 
 Backtab, but not Tab.
 
 
 kkeysequencewidget.cpp however has special handling (which kglobaccel 
 would have to adapt, but this is a total mess)
 ```
if ((keyQt == Qt::Key_Backtab)  (d-modifierKeys  
 Qt::SHIFT)) {
 0758 keyQt = Qt::Key_Tab | d-modifierKeys;
 0759 } else if (KKeyServer::isShiftAsModifierAllowed(keyQt)) {
 0760 keyQt |= d-modifierKeys;
 ```
 
 Yuxuan Shui wrote:
 Any suggestions how to fix this?

basically by ignoring Qt::Key_Tab | Qt::SHIFT and map Qt::Key_Backtab to it 
(at least this should be what is grabbed)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123376/#review83734
---


On April 25, 2015, 7:54 nachm., Yuxuan Shui wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123376/
 ---
 
 (Updated April 25, 2015, 7:54 nachm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin

Re: Review Request 123376: Fix handling of mod-shift-number shortcuts.

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123376/#review83734
---


bump, we should get this fixed for 5.4

Shui, do you want to work on improving the patch and adding the testcase?

- Thomas Lübking


On April 25, 2015, 7:54 nachm., Yuxuan Shui wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123376/
 ---
 
 (Updated April 25, 2015, 7:54 nachm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Bugs: 341959
 https://bugs.kde.org/show_bug.cgi?id=341959
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 For details, see discussion in corresponding bug report.
 
 
 Diffs
 -
 
   src/runtime/kglobalaccel_x11.cpp abee5bc 
 
 Diff: https://git.reviewboard.kde.org/r/123376/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Yuxuan Shui
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123376: Fix handling of mod-shift-number shortcuts.

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123376/#review79753
---


Two things.
a) 1 is a magic number (stupid xcb, should be xcppb ;-) - please add a 
comment that this picks the 1st level shift
b) the overall code looks a bit inefficient. No idea how smart xcb is under the 
hood, but
   1. we fetch the keysym via xcb_key_press_lookup_keysym
   2. we check the mod state to be numlock
   3. we fetch the keysym again (xcb_key_press_lookup_keysym just wraps 
xcb_key_symbols_get_keysym on ev-detail)
   4. we map keysymX to keysymQt
   5. we check the (mapped to Qt) modifier for the SHIFT flag
   6. this patch fetches and maps keysymX once more
 do you want to do a general cleanup with this or another patch?

- Thomas Lübking


On April 25, 2015, 7:54 nachm., Yuxuan Shui wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123376/
 ---
 
 (Updated April 25, 2015, 7:54 nachm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Bugs: 341959
 https://bugs.kde.org/show_bug.cgi?id=341959
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 For details, see discussion in corresponding bug report.
 
 
 Diffs
 -
 
   src/runtime/kglobalaccel_x11.cpp abee5bc 
 
 Diff: https://git.reviewboard.kde.org/r/123376/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Yuxuan Shui
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122981: add new method KGlobalAccel::shortcut(componentName, actionId)

[Thomas Lübking]

 On April 16, 2015, 3:20 nachm., Martin Gräßlin wrote:
  src/kglobalaccel.h, line 276
  https://git.reviewboard.kde.org/r/122981/diff/5/?file=360654#file360654line276
 
  I think it needs a different name, see:
  
  https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#The_Do.27s_and_Don.27ts
  
  in the don't section
  
add an overload (BC, but not SC: makes func ambiguous), adding 
  overloads to already overloaded functions is ok (any use of func already 
  needed a cast).

What would be only relevant if we assume this to ever have become a function 
pointer target...

However, globalShortcut seems consistent enough ;-)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122981/#review79060
---


On April 10, 2015, 11:04 vorm., Gregor Mi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122981/
 ---
 
 (Updated April 10, 2015, 11:04 vorm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 Introduce new method: QListQKeySequence KGlobalAccel::shortcut(const 
 QString componentName, const QString actionId) to retrieve shortcut list 
 from global settings.
 
 See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: 
 add Kill Window to End Process button and show correct keyboard shortcut).
 
 
 Diffs
 -
 
   autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da 
   src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 
   src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 
   src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e 
   autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6 
 
 Diff: https://git.reviewboard.kde.org/r/122981/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Gregor Mi
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122981: add new method KGlobalAccel::shortcut(componentName, actionId)

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122981/#review78777
---



src/kglobalaccel.cpp (line 274)
https://git.reviewboard.kde.org/r/122981/#comment53905

*cough*


- Thomas Lübking


On April 10, 2015, 11:04 vorm., Gregor Mi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122981/
 ---
 
 (Updated April 10, 2015, 11:04 vorm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 Introduce new method: QListQKeySequence KGlobalAccel::shortcut(const 
 QString componentName, const QString actionId) to retrieve shortcut list 
 from global settings.
 
 See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: 
 add Kill Window to End Process button and show correct keyboard shortcut).
 
 
 Diffs
 -
 
   autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da 
   src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 
   src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 
   src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e 
   autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6 
 
 Diff: https://git.reviewboard.kde.org/r/122981/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Gregor Mi
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122981: add KGlobalAccel::loadShortcutFromGlobalSettings

[Thomas Lübking]

 On April 4, 2015, 9:14 vorm., Gregor Mi wrote:
  src/kglobalaccel.h, line 372
  https://git.reviewboard.kde.org/r/122981/diff/4/?file=359983#file359983line372
 
  NOTE that this causes compile error in libKF5XmlGui
  
  
  /home/gregor/dev/kf5/usr/lib64/libKF5XmlGui.so.5.8.0: undefined 
  reference to `KGlobalAccel::globalShortcutChanged(QAction*, QKeySequence 
  const)'
  collect2: error: ld returned 1 exit status

Of course not - that's an ABI *and* API incompatible change (big no-go, not 
gonna happen)

a) introduce a flag to ::updateGlobalShortcut() whether the shortcut should 
actually be updated (or just somehow initialized) by shortcut() (defaulting 
true)
b) if that flag is false (set by shortcut() on invocation)
   1. exit early if there's a known shortcut to skip the expensive stuff
   2. NEVER emit the changed signal
c) const_castQAction* the action parameter from ::shortcut()
d) add comments that constness is to be (weakly) guaranteed if onlyInitial 
(or so) is true
e) add a TODO for KF/6 to remove constness from the ::shortcut() parameter


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122981/#review78472
---


On April 3, 2015, 5:50 nachm., Gregor Mi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122981/
 ---
 
 (Updated April 3, 2015, 5:50 nachm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 In some cases you need to call loadShortcutFromGlobalSettings() in order not 
 to get a an empty list when calling shortcut() (which is const).
 
 See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: 
 add Kill Window to End Process button and show correct keyboard shortcut).
 
 
 Diffs
 -
 
   autotests/kglobalshortcuttest.h b1122a8f5ca2f3f7afbe78f8edba87325426c1a6 
   autotests/kglobalshortcuttest.cpp 3b661bbb612807a3bbbe34835d4ae712c2ec74da 
   src/kglobalaccel.h 3fe20ca8e4ec6ceb0bb9e54235aef7f1aeeb8c16 
   src/kglobalaccel.cpp 1b6b3f5cb6d42401d684e6a491d12a6e57248fd1 
   src/kglobalaccel_p.h eca7c52378ad60d0d5806561214b9788dd46a11e 
 
 Diff: https://git.reviewboard.kde.org/r/122981/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Gregor Mi
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122981: add KGlobalAccel::loadShortcutFromGlobalSettings

[Thomas Lübking]

 On März 22, 2015, 3:56 nachm., Albert Astals Cid wrote:
  src/kglobalaccel.h, line 260
  https://git.reviewboard.kde.org/r/122981/diff/2/?file=356138#file356138line260
 
  typo in following
 
 Gregor Mi wrote:
 About why it is needed: see 
 KGlobalShortcutTest::testLoadShortcutFromGlobalSettings: shortcut() does not 
 behave as expected when the new method loadShortcutFromGlobalSettings() is 
 not called
 
 I don't know if always calling loadShortcutFromGlobalSettings() before 
 shortcut() would break existing code. E.g. in 
 kwin/effects/desktopgrid/desktopgrid_config.cpp shortcut() is used without 
 prior call of loadShortcutFromGlobalSettings().
 
 On the other hand, a call to loadShortcutFromGlobalSettings() might be 
 harmless but I don't have a good overview of the KGlobalAccel usage patterns.
 
 Albert Astals Cid wrote:
 Honestly i have no idea on how this kglobalaccel thing works, but it 
 seems to me you're just adding a way to workaround a bug instead of fixing it.
 
 Gregor Mi wrote:
 Me neither :-)
 
 I was hoping that someone with more experience with kglobalaccel can say 
 more about the proposed solution.
 
 Like you Thomas suggested to put the loading method into shortcut() 
 (which is essentially a one-liner: d-updateGlobalShortcut(action, 
 KGlobalAccelPrivate::ActiveShortcut, KGlobalAccel::Autoloading);).
 
 Martin Gräßlin wrote:
 well we could try it: add the one-line change and try whether the unit 
 tests pass and then try whether the case in DesktopGridConfig works.
 
 Unfortunately I'm also not that into KGloalAccel code yet to be 100 % 
 whether it's a valid change :-(
 
 Gregor Mi wrote:
 I tried to call loadShortcutFromGlobalSettings() from within shortcut() 
 and found that this violates const constraints:
 
 shortcut() is const
 d-updateGlobalShortcut is not const
 
 So this is not possible.
 
 How to proceed now?
 
 Albert Astals Cid wrote:
 make mutable the members that updateGlobalShortcut modifies
 
 Gregor Mi wrote:
 I tried again and the call was even possible with only introducing some 
 more consts. But the call of loadShortcutFromGlobalSettings() from within 
 shortcut() makes more unit tests fail than without.
 
 = the general call of loadShortcutFromGlobalSettings() from within 
 shortcut() seems to harm more than it helps.
 
 Currently, I have a problem with getting the unit tests to all pass 
 (which is independet of this change; it happens also in master branch). See 
 my analysis below and comment of the unit test itself:
 
 /* These tests could be better. They don't include actually triggering 
 actions,
and we just choose very improbable shortcuts to avoid conflicts with 
 real
applications' shortcuts. */

 It would be nice to document some instructions of how to reset the global 
 shortcuts to a defined state.
 
 Albert Astals Cid wrote:
 Use strace to see which files are being read.
 
 Gregor Mi wrote:
 I did
 ```
 strace -o strace1.txt ./kglobalshortcuttest testFindActionByKey
 sed -n '/open/p' strace1.txt  strace1.open.txt
 ```
 
 This is the complete output http://paste.opensuse.org/97828 and filtered 
 by everything that contains open: http://paste.opensuse.org/54497725.
 
 List all filenames:
 `sed -n 's/open((.*).*/\1/p' strace1.open.txt  strace1.files.txt`
 
 Remove duplicates:
 `sort strace1.files.txt | uniq -u  strace1.files.nodups.txt`
 
 Manually remove /dev/urandom from that list.
 
 Grep for testLoadShortcutFromGlobalSettings in the list.
 ```
 for f in `cat strace1.files.nodups.txt`; do grep 
 testLoadShortcutFromGlobalSettings $f; done
 ```
 
 No result.
 
 The shortcuts seem to be retrieved via DBUS. Is this covered by strace as 
 well?
 
 Albert Astals Cid wrote:
 No, strace doesn't cover dbus.

But dbus-monitor does ;-)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122981/#review77912
---


On April 3, 2015, 5:50 nachm., Gregor Mi wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122981/
 ---
 
 (Updated April 3, 2015, 5:50 nachm.)
 
 
 Review request for KDE Frameworks, Martin Gräßlin and Thomas Lübking.
 
 
 Repository: kglobalaccel
 
 
 Description
 ---
 
 In some cases you need to call loadShortcutFromGlobalSettings() in order not 
 to get a an empty list when calling shortcut() (which is const).
 
 See discussion in https://git.reviewboard.kde.org/r/122249/ (libksysguard: 
 add Kill Window to End Process button and show correct

Re: Review Request 121020: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121020/
---

(Updated Feb. 6, 2015, 4:26 nachm.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: plasma-desktop


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285t=123587
https://git.reviewboard.kde.org/r/121019/
https://git.reviewboard.kde.org/r/121021/


Diffs
-

  kcms/krdb/krdb.cpp 8452aa5 
  kcms/style/kcmstyle.cpp 9a13f45 

Diff: https://git.reviewboard.kde.org/r/121020/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121021: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121021/
---

(Updated Feb. 6, 2015, 4:26 nachm.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: kdelibs4support


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285t=123587
https://git.reviewboard.kde.org/r/121019/
https://git.reviewboard.kde.org/r/121020/


Diffs
-

  src/kdeui/k4style.cpp a1a2ab1 
  src/kdeui/kglobalsettings.h d63ac69 

Diff: https://git.reviewboard.kde.org/r/121021/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121019: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121019/
---

(Updated Feb. 6, 2015, 4:25 nachm.)


Status
--

This change has been discarded.


Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: frameworkintegration


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285t=123587
https://git.reviewboard.kde.org/r/121020/
https://git.reviewboard.kde.org/r/121021/


Diffs
-

  autotests/kdeplatformtheme_changed_kdeglobals 910e0e3 
  autotests/kdeplatformtheme_kdeglobals df52410 
  src/kstyle/kstyle.cpp b5f7363 
  src/platformtheme/khintssettings.cpp 8799216 

Diff: https://git.reviewboard.kde.org/r/121019/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122198: Add support for _NET_WM_OPAQUE_REGION

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122198/#review74617
---

Ship it!


I assume there's requirement for a setter as well (rather than even a getter)?

- Thomas Lübking


On Jan. 22, 2015, 1:49 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122198/
 ---
 
 (Updated Jan. 22, 2015, 1:49 nachm.)
 
 
 Review request for KDE Frameworks and kwin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 NETWinInfo provides the opaque region as a vector of NETRect.
 
 CHANGELOG: support for _NET_WM_OPAQUE_REGION
 
 
 Diffs
 -
 
   src/netwm.cpp 600b0f6edbd42dd57f9bc2c1ce5dbc91844d344d 
   src/netwm_def.h 3066726699947b3415433107e88a049584f3ebbc 
   src/netwm_p.h e709caf16481938bb9f93083139ec3ea49e0bcb7 
   autotests/netrootinfotestwm.cpp c88ba0a10555181b8b80c9156e587185455d5047 
   autotests/netwininfotestclient.cpp cebf93b50a8917d243523269ecfe117f343d7f30 
   src/netwm.h 7729294286b3ec6dc94684ffde36b32eace7ae0d 
 
 Diff: https://git.reviewboard.kde.org/r/122198/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122086: Add new method KWindowSystem::icons

[Thomas Lübking]

 On Jan. 21, 2015, 9:07 vorm., Martin Gräßlin wrote:
  ping - do we want this approach to go in?

I've two concerns about this.
1. the name icons implies several (it's unfortunate that ::icon isn't 
::iconPixmap) - maybe windowIcon() or similar
2. the behavior how an icon is build up from multiple sources (isn't)
   right now, you could end up w/ one 16x16 NETWM icon pixmap.
   Considerations: NETWM and WM_HINTS could be more specific than an icon from 
the appname, the appname could though be more size complete and finally 
WM_HINTS would likely be ugly compared to the other two (bitmasked). Overmore 
it might be undesirable to end up w/ an icon that has completely different 
looks for different sizes.
   As we don't have a specific hint on the required target size, it gets harder 
to ever take an informed decision on what to return here, but one might want to 
at least check the sizes provided by NEWTM before returning early (if other 
sources are allowed by flags) - yes, I know that this is not a straight 
suggestion for improvement :-(


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122086/#review74460
---


On Jan. 21, 2015, 9:06 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122086/
 ---
 
 (Updated Jan. 21, 2015, 9:06 vorm.)
 
 
 Review request for KDE Frameworks and kwin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 ::icons tries to retrieve all available icon sizes and combine them
 into a single QIcon. On the X11 platform this can significantly
 reduce the time needed to fetch all available icons compared to the
 already existing ::icon methods which return a QPixmap of a specific
 size.
 
 The change includes a new test application icontest which shows all
 available icons in all available sizes for a window id passed as
 command line argument.
 
 CHANGELOG: New method QIcon KWindowSystem::icons(WId win, int flags)
 
 
 Diffs
 -
 
   src/kwindowsystem.h 322322f12dda7279567be8420533ed22cd52 
   src/kwindowsystem.cpp 65d215b6dfbf4df22e871fd7187fface75abb61b 
   src/kwindowsystem_p.h 1f01145b5c7efe925fcb8242f92af17b704e01c9 
   src/kwindowsystem_p_x11.h 0d4b6ba551776d2fa08030f78226ecdb3c80c889 
   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
   tests/CMakeLists.txt ce68cc505a69ea9a3cf645e9ae587bd89abe1648 
   tests/icontest.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/122086/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122140: Support reading IconPixmap and IconMask from WM_HINTS in NETWinInfo

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122140/#review74333
---

Ship it!



src/netwm.h
https://git.reviewboard.kde.org/r/122140/#comment51587

icccmIconPixmap()  and a comment to in doubt preferably use the 
::icon(int, int) function (for this is a legacy feature w/ reduced 
functionality)?


- Thomas Lübking


On Jan. 19, 2015, 11:03 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122140/
 ---
 
 (Updated Jan. 19, 2015, 11:03 vorm.)
 
 
 Review request for KDE Frameworks and kwin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 WM_HINTS property is already read and this provides the icon pixmap
 and icon mask hint as read from WM_HINTS. This can be used for example
 in KWindowSystem::icon which currently uses an XLib call.
 
 Use NETWinInfo's WM2WindowClass and WM2IconPixmap in 
 KWindowSystemPrivateX11::icon
 
 Instead of using an XLib call the wrapper from NETWinInfo is used.
 This reduces the number of roundtrips to the X server in case flags
 includes both NETWM, ClassHint and WMHints.
 
 In additon we don't need the deprecated x error eater any more.
 
 
 Diffs
 -
 
   autotests/netwininfotestclient.cpp a0fdc403bb8329ea9fde786494bc0ccf88a8ebfd 
   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
   src/netwm.h 382ab460867905fdf0a4106a271e6614478fb438 
   src/netwm.cpp 1c96aaf9e480842c0b5379c89722f2e91a664476 
   src/netwm_def.h 49d02035eaec402a61e0b17b377b27c80d605b7b 
   src/netwm_p.h 0f1dd62ecd9c856e028dd33d28d461ed7099f60b 
 
 Diff: https://git.reviewboard.kde.org/r/122140/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122144: Add new overload to KWindowSystem::icon

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122144/#review74335
---

Ship it!


Ship It!

- Thomas Lübking


On Jan. 19, 2015, 1:45 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122144/
 ---
 
 (Updated Jan. 19, 2015, 1:45 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 The new overload is a special solution for the X11 platform. It takes
 the NETWinInfo as argument to read the information from.
 
 This significantly reduces the time to fetch icons for users who
 already have a NETWinInfo with the required data. E.g. for the case
 of KWin it can reduce the number of roundtrips to the X Server from
 up to 15 to 0.
 
 For non-X11 platforms the method just delegates to the method not
 taking the NETWinInfo argument.
 
 CHANGELOG: New overload to KWindowSystem::icon which reduces roundtrips to 
 X-Server
 
 
 Diffs
 -
 
   src/kwindowsystem_p_x11.h 0d4b6ba551776d2fa08030f78226ecdb3c80c889 
   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
   src/kwindowsystem.h 322322f12dda7279567be8420533ed22cd52 
   src/kwindowsystem.cpp 65d215b6dfbf4df22e871fd7187fface75abb61b 
 
 Diff: https://git.reviewboard.kde.org/r/122144/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122086: Add new method KWindowSystem::icons

[Thomas Lübking]

 On Jan. 16, 2015, 4:13 nachm., Thomas Lübking wrote:
  Wrt the other patches, using NETIcon over there, the requirement in KWin 
  and likely libtaskbar (ie. keep this in sync):
  What do you think about extending the API by allowing to optionally feed in 
  the required elements?
  
  QIcon KWindowSystem::completeIcon(WId win, int flags, NETWinInfo *nwi = 
  nullptr, XWMHints *xhints = nullptr, QString className = QString());
  
  The function would then (in case nwi is supplied) check passedProperties() 
  to see whether icons and/or classname can be fetched from there (unless 
  classname is explicitly trumped by the last parameter)
 
 Eike Hein wrote:
 Regarding keeping things in sync: I've prepared a changeset to 
 libtaskmanager that deletes around 90% of stunningly redundant icon code and 
 will replace it with a call to KWindowSystem::icons(). It won't pass the 
 ClassHint flag though to avoid an additional XGetClassHint in KWS since it 
 has the ClassHint already, so it will do the fallback itself.
 
 Thomas Lübking wrote:
 Ie. you'd benefit from such extended API?
 
 Do you have a NETWindowInfo or the XWMHints around as well (for a 
 different purpose, like the input flag or the window group)?
 
 Martin Gräßlin wrote:
  QIcon KWindowSystem::completeIcon(WId win, int flags, NETWinInfo nwi = 
 nullptr, XWMHints xhints = nullptr, QString className = QString());
 
 I thought about it when implementing it, but I fear we cannot do it as 
 NETWinInfo is only available on X11. So it's unsuited for the cross-platform 
 API. Though maybe one could do something with forward declaring it. XWMHints 
 and the classname shouldn't be needed as both should be possible to take from 
 NETWinInfo (at least we have a replacement for XWMHints in NETWinInfo, we 
 just need to add reading out the pixmap hint).

#if Q_OS_USELESS
typedef NETWinInfo void;
#endif

/** NETWinInfo  XWMHints are ignored on non-X11 systems - and so is likely 
className */

kwindowsystem_useless_os.cpp
QIcon KWindowSystem::completeIcon(WId win, int flags, NETWinInfo nwi = nullptr, 
XWMHints xhints = nullptr, QString className = QString())
{
   Q_UNUSED(nwi);
   Q_UNUSED(hints);
   ...
}


Ok, maybe I should have read the third sentence as well ;-)

About the classname:
I meant to be able to override the plain one, resp. to pass one despite not 
having a sufficient NETWinInfo (so it doesn't have to be fetched again)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122086/#review74142
---


On Jan. 16, 2015, 12:55 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122086/
 ---
 
 (Updated Jan. 16, 2015, 12:55 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 ::icons tries to retrieve all available icon sizes and combine them
 into a single QIcon. On the X11 platform this can significantly
 reduce the time needed to fetch all available icons compared to the
 already existing ::icon methods which return a QPixmap of a specific
 size.
 
 The change includes a new test application icontest which shows all
 available icons in all available sizes for a window id passed as
 command line argument.
 
 CHANGELOG: New method QIcon KWindowSystem::icons(WId win, int flags)
 
 
 Diffs
 -
 
   src/kwindowsystem.h 322322f12dda7279567be8420533ed22cd52 
   src/kwindowsystem.cpp 65d215b6dfbf4df22e871fd7187fface75abb61b 
   src/kwindowsystem_p.h 1f01145b5c7efe925fcb8242f92af17b704e01c9 
   src/kwindowsystem_p_x11.h 0d4b6ba551776d2fa08030f78226ecdb3c80c889 
   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
   tests/CMakeLists.txt ce68cc505a69ea9a3cf645e9ae587bd89abe1648 
   tests/icontest.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/122086/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122086: Add new method KWindowSystem::icons

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122086/#review74142
---


Wrt the other patches, using NETIcon over there, the requirement in KWin and 
likely libtaskbar (ie. keep this in sync):
What do you think about extending the API by allowing to optionally feed in the 
required elements?

QIcon KWindowSystem::completeIcon(WId win, int flags, NETWinInfo *nwi = 
nullptr, XWMHints *xhints = nullptr, QString className = QString());

The function would then (in case nwi is supplied) check passedProperties() to 
see whether icons and/or classname can be fetched from there (unless classname 
is explicitly trumped by the last parameter)

- Thomas Lübking


On Jan. 16, 2015, 12:55 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122086/
 ---
 
 (Updated Jan. 16, 2015, 12:55 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 ::icons tries to retrieve all available icon sizes and combine them
 into a single QIcon. On the X11 platform this can significantly
 reduce the time needed to fetch all available icons compared to the
 already existing ::icon methods which return a QPixmap of a specific
 size.
 
 The change includes a new test application icontest which shows all
 available icons in all available sizes for a window id passed as
 command line argument.
 
 CHANGELOG: New method QIcon KWindowSystem::icons(WId win, int flags)
 
 
 Diffs
 -
 
   src/kwindowsystem.h 322322f12dda7279567be8420533ed22cd52 
   src/kwindowsystem.cpp 65d215b6dfbf4df22e871fd7187fface75abb61b 
   src/kwindowsystem_p.h 1f01145b5c7efe925fcb8242f92af17b704e01c9 
   src/kwindowsystem_p_x11.h 0d4b6ba551776d2fa08030f78226ecdb3c80c889 
   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
   tests/CMakeLists.txt ce68cc505a69ea9a3cf645e9ae587bd89abe1648 
   tests/icontest.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/122086/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 122086: Add new method KWindowSystem::icons

[Thomas Lübking]

 On Jan. 16, 2015, 4:13 nachm., Thomas Lübking wrote:
  Wrt the other patches, using NETIcon over there, the requirement in KWin 
  and likely libtaskbar (ie. keep this in sync):
  What do you think about extending the API by allowing to optionally feed in 
  the required elements?
  
  QIcon KWindowSystem::completeIcon(WId win, int flags, NETWinInfo *nwi = 
  nullptr, XWMHints *xhints = nullptr, QString className = QString());
  
  The function would then (in case nwi is supplied) check passedProperties() 
  to see whether icons and/or classname can be fetched from there (unless 
  classname is explicitly trumped by the last parameter)
 
 Eike Hein wrote:
 Regarding keeping things in sync: I've prepared a changeset to 
 libtaskmanager that deletes around 90% of stunningly redundant icon code and 
 will replace it with a call to KWindowSystem::icons(). It won't pass the 
 ClassHint flag though to avoid an additional XGetClassHint in KWS since it 
 has the ClassHint already, so it will do the fallback itself.

Ie. you'd benefit from such extended API?

Do you have a NETWindowInfo or the XWMHints around as well (for a different 
purpose, like the input flag or the window group)?


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122086/#review74142
---


On Jan. 16, 2015, 12:55 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122086/
 ---
 
 (Updated Jan. 16, 2015, 12:55 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 ::icons tries to retrieve all available icon sizes and combine them
 into a single QIcon. On the X11 platform this can significantly
 reduce the time needed to fetch all available icons compared to the
 already existing ::icon methods which return a QPixmap of a specific
 size.
 
 The change includes a new test application icontest which shows all
 available icons in all available sizes for a window id passed as
 command line argument.
 
 CHANGELOG: New method QIcon KWindowSystem::icons(WId win, int flags)
 
 
 Diffs
 -
 
   src/kwindowsystem.h 322322f12dda7279567be8420533ed22cd52 
   src/kwindowsystem.cpp 65d215b6dfbf4df22e871fd7187fface75abb61b 
   src/kwindowsystem_p.h 1f01145b5c7efe925fcb8242f92af17b704e01c9 
   src/kwindowsystem_p_x11.h 0d4b6ba551776d2fa08030f78226ecdb3c80c889 
   src/kwindowsystem_x11.cpp bf958ae63b48424fc412405259f082b740928f9a 
   tests/CMakeLists.txt ce68cc505a69ea9a3cf645e9ae587bd89abe1648 
   tests/icontest.cpp PRE-CREATION 
 
 Diff: https://git.reviewboard.kde.org/r/122086/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121459: Fixed data type for _KDE_NET_WM_BLUR_BEHIND_REGION

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121459/#review71877
---

Ship it!


using long should have been wrong itfp, since it's only 32bit on i32 and LLP64 
(- what MS calls an OS ;-), but not on ILP64 or LP64 (- typical unixoid), ie. 
the length of long may easily vary between client and server (or two clients)

See http://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models

- Thomas Lübking


On Dez. 12, 2014, 10:22 vorm., Hugo Pereira Da Costa wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121459/
 ---
 
 (Updated Dez. 12, 2014, 10:22 vorm.)
 
 
 Review request for KDE Frameworks and kwin.
 
 
 Repository: kwin
 
 
 Description
 ---
 
 Somehow (although I do not know the details), the change from X calls to xcb 
 (or similarily XA_CARDINAL to XCB_ATOM_CARDINAL) involves a conversion from 
 unsigned int to uint32_t.
 As a consequence one must change the data type and cast accordingly, so that 
 'valid' blur regions passed from Qt4/KDE4 applications and that where 
 properly honored by kwin@kde4 are still working for kwin@kf5
 
 thanks
 
 
 Diffs
 -
 
   effects/blur/blur.cpp 29852de 
 
 Diff: https://git.reviewboard.kde.org/r/121459/diff/
 
 
 Testing
 ---
 
 yes
 
 BLUR_BEHIND_REGIONS that were honored with kde4 and not any more with kf5, 
 are again ...
 
 I have no clue whether this impacts plasma@kf5
 (it could well be that there the property was fixed 'the other way around', 
 by casting from unsigned long to uint_32 when setting the property, via XCB. 
 This however would break when running against kwin@kde4)
 
 
 Thanks,
 
 Hugo Pereira Da Costa
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121390: make Qt5 theme build on Linux again

[Thomas Lübking]

 On Dez. 8, 2014, 2:07 nachm., Martin Gräßlin wrote:
  this is wrong - please have a look at various frameworks on how to do it 
  properly. In the end it should be:
  #if HAVE_X11
  // x11 specific stuff
  #endif
  
  obviously it also needs a runtime check:
  if (QX11Info::isPlatformX11())
  
  as we no longer should assume that X11 is the only platform on Unix(non 
  OSX).
 
 René J.V. Bertin wrote:
 I found a reference to HAVE_X11 online, but that token is not defined. 
 Note also that the Qt5 theme is supposed to build without KF5 too, for pure 
 Qt5 applications, so this kind of token should rather be provided by the Qt 
 cmake files rather than KDE's.
 
 I'll leave the runtime checks to the QtCurve devs, as they obviously 
 should be made in lots of locations and it's their call. I myself don't see 
 the point in doing a runtime check whether a platform is X11, though. It's 
 known at build time if a platform is X11. Doing a runtime check before each 
 theming action if `Q11Info::isX11Active()` (or some such call) seems to be an 
 expensive concession to a rather improbable set-up. If distros really decide 
 to give the user a choice between X11 and Wayland at login ... let them 
 figure out how to streamline that first, and then add runtime checks for the 
 active graphics backend where really needed...
 (In fact, I myself would avoid anything tied to the display layer as much 
 as possible; the fact I had to go back in a few months after the previous 
 porting effort goes to show how easy it is to break builds on other platforms 
 with that kind of functionality - as if my own error wasn't enough already.)
 
 Martin Gräßlin wrote:
 HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set 
 manually depending on whether the source is built with or without X11 support.
 
 Concerning the runtime check:
 kwrite -platform wayland
 
 and your app is going to crash like hell if there is no runtime checks.
 
 René J.V. Bertin wrote:
 ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland
 This application failed to start because it could not find or load the Qt 
 platform plugin wayland.
 
 Available platform plugins are: linuxfb, minimal, offscreen, xcb.
 
 Reinstalling the application may fix this problem.
 Abort (core dumped)
 ```
 
 Right, so with runtime checks it doesn't crash, it just self-destructs. 
 Very useful difference :)
 Of course an application will crash if it tries to use an unavailable 
 displaying method, or if the linker tries to load shared libraries that 
 aren't present. In fact, with X11 it might actually exit nicely with a 
 message about a display rather than crash.
 
 This just underlines my point: the only use for runtime checks in this 
 context if is the same binaries are supposed to work with multiple displaying 
 backends (or platform plugins). Whether QtCurve ought to support that is a 
 call for its developers to make, like I said above. The only way to do that 
 properly without (too much) overhead is to do the check at initialisation 
 time rather than preceding each backend-specific call, i.e. use 
 functionpointers or some OO/C++ alternative. I don't know how preferable 
 Wayland is to X11; without that I see only an interest for people working on 
 Wayland development under X11 for this kind of runtime switch support.
 To put this into context: I've often thought how it'd be nice if Qt-mac 
 would be able to use X11, but I've always dismissed the possibility that that 
 might be a runtime switch, exactly because it would introduce too much 
 overhead and/or complexity for a feature that'd be used only rarely.

 Right, so with runtime checks it doesn't crash, it just self-destructs. 

You're missing the point entirely. The compile time checks have no implication 
on the runtime environment.
Of course you cannot use the wayland platform plugin if it's not available, but 
you *can* compile Qt/KDE w/ X11 and wayland present - but making X11 calls when 
running on the wayland PP will crash the application - thus you must check 
whether you're operating on X11/xdg at runtime.

Also testing for UNIX but not OSX to make X11 calls is plain wrong. Could be 
framebuffer console or wayland and no X11 installed at all.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121390/#review71553
---


On Dez. 8, 2014, 1:38 nachm., René J.V. Bertin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121390/
 ---
 
 (Updated Dez. 8, 2014, 1:38 nachm.)
 
 
 Review request for KDE Frameworks, Qt KDE and Yichao Yu.
 
 
 Repository: qtcurve
 

Re: Review Request 121390: make Qt5 theme build on Linux again

[Thomas Lübking]

 On Dez. 8, 2014, 2:07 nachm., Martin Gräßlin wrote:
  this is wrong - please have a look at various frameworks on how to do it 
  properly. In the end it should be:
  #if HAVE_X11
  // x11 specific stuff
  #endif
  
  obviously it also needs a runtime check:
  if (QX11Info::isPlatformX11())
  
  as we no longer should assume that X11 is the only platform on Unix(non 
  OSX).
 
 René J.V. Bertin wrote:
 I found a reference to HAVE_X11 online, but that token is not defined. 
 Note also that the Qt5 theme is supposed to build without KF5 too, for pure 
 Qt5 applications, so this kind of token should rather be provided by the Qt 
 cmake files rather than KDE's.
 
 I'll leave the runtime checks to the QtCurve devs, as they obviously 
 should be made in lots of locations and it's their call. I myself don't see 
 the point in doing a runtime check whether a platform is X11, though. It's 
 known at build time if a platform is X11. Doing a runtime check before each 
 theming action if `Q11Info::isX11Active()` (or some such call) seems to be an 
 expensive concession to a rather improbable set-up. If distros really decide 
 to give the user a choice between X11 and Wayland at login ... let them 
 figure out how to streamline that first, and then add runtime checks for the 
 active graphics backend where really needed...
 (In fact, I myself would avoid anything tied to the display layer as much 
 as possible; the fact I had to go back in a few months after the previous 
 porting effort goes to show how easy it is to break builds on other platforms 
 with that kind of functionality - as if my own error wasn't enough already.)
 
 Martin Gräßlin wrote:
 HAVE_X11 is neither defined by Qt5 nor by KF5. It needs to be set 
 manually depending on whether the source is built with or without X11 support.
 
 Concerning the runtime check:
 kwrite -platform wayland
 
 and your app is going to crash like hell if there is no runtime checks.
 
 René J.V. Bertin wrote:
 ``` neon5-exec /opt/project-neon5/bin/kwrite -platform wayland
 This application failed to start because it could not find or load the Qt 
 platform plugin wayland.
 
 Available platform plugins are: linuxfb, minimal, offscreen, xcb.
 
 Reinstalling the application may fix this problem.
 Abort (core dumped)
 ```
 
 Right, so with runtime checks it doesn't crash, it just self-destructs. 
 Very useful difference :)
 Of course an application will crash if it tries to use an unavailable 
 displaying method, or if the linker tries to load shared libraries that 
 aren't present. In fact, with X11 it might actually exit nicely with a 
 message about a display rather than crash.
 
 This just underlines my point: the only use for runtime checks in this 
 context if is the same binaries are supposed to work with multiple displaying 
 backends (or platform plugins). Whether QtCurve ought to support that is a 
 call for its developers to make, like I said above. The only way to do that 
 properly without (too much) overhead is to do the check at initialisation 
 time rather than preceding each backend-specific call, i.e. use 
 functionpointers or some OO/C++ alternative. I don't know how preferable 
 Wayland is to X11; without that I see only an interest for people working on 
 Wayland development under X11 for this kind of runtime switch support.
 To put this into context: I've often thought how it'd be nice if Qt-mac 
 would be able to use X11, but I've always dismissed the possibility that that 
 might be a runtime switch, exactly because it would introduce too much 
 overhead and/or complexity for a feature that'd be used only rarely.
 
 Thomas Lübking wrote:
  Right, so with runtime checks it doesn't crash, it just self-destructs. 
 
 You're missing the point entirely. The compile time checks have no 
 implication on the runtime environment.
 Of course you cannot use the wayland platform plugin if it's not 
 available, but you *can* compile Qt/KDE w/ X11 and wayland present - but 
 making X11 calls when running on the wayland PP will crash the application - 
 thus you must check whether you're operating on X11/xdg at runtime.
 
 Also testing for UNIX but not OSX to make X11 calls is plain wrong. 
 Could be framebuffer console or wayland and no X11 installed at all.
 
 Martin Gräßlin wrote:
 for more information please see my blog post: 
 http://blog.martin-graesslin.com/blog/2014/02/running-frameworks-powered-applications-on-wayland/
 
 Btw. the QtWayland PPA will be available starting with Qt 5.4 - a version 
 I'm already using.
 
 René J.V. Bertin wrote:
 @Thomas: we're not talking about compile time checks. Those evidently 
 don't have any implication on the runtime environment (if done correctly :)).
 I guess my point is simply that the fact that you can (= it's possible 
 to) compile Qt/KDE with every conceivable display

Re: Review Request 121299: Add NET::OSD window type

[Thomas Lübking]

 On Nov. 30, 2014, 10:45 nachm., Thomas Lübking wrote:
  Please notice that override redirects are above EVERY managed window, ie. 
  if you fullscreen window happens to be an SDL(? some toolkit does this at 
  least) game, the new layer will fail its job.
  
  Random 3¢:
  So either this is superfluous (the window should just be override redirect) 
  or (reg. wayland and unmanaged clients) in NETWM (to get away from 
  unmanaged clients in general)
  
  Because that re-appeared over and over again lately:
  I'd pesonally oppose this as but we always forget to make new QML stuff 
  override redirect specific solution.
  
  Otoh, if this actually was a canonical type w/ well defined behavior, we 
  could make fine-grained adjustments, eg. have it not above keep-above 
  fullscreens or similar.
 
 Martin Gräßlin wrote:
 it was a suggestion by me to introduce a new type. The problem is that we 
 mixed up notification and osd. That is I implemented support for notification 
 and thought that would be for notification, but it got used for OSD.
 
 Now the initial idea on why I wanted to have it with a dedicated type 
 still holds: having more information in KWin about the window type. That's a 
 good thing in general. So even if it were just an override redirect it should 
 have a type to indicate it's type. We will need that on Wayland anyway that 
 windows must provide more information than currently (no override redirect 
 there).
 
 Concerning the games which use override-redirect: I don't think we should 
 design our API around the brokeness of games.

I'm certainly ok w/ extending and fine-graining the window types, but would 
prefer to have it in NETWM[1] (instead of having a proprietary feature and 
later on possible collision/confusion), notably because:

```
_NET_WM_WINDOW_TYPE_NOTIFICATION indicates a notification. An example of a 
notification would be a bubble appearing with informative text such as Your 
laptop is running out of power etc. This property is typically used on 
override-redirect windows.
```

bubble ... with informative text ... typically used on override-redirect 
windows sounds *a lot* like an OSD.
So it needs proper distinction, resp., if this is for very specific 
notifications (volume slider), maybe just a specification that managed 
keep-above notifications are supposed to be in the active layer (thus on top of 
fullscreen windows, unless those are flagged keepabove as well - i don't want a 
volume hint when an imp runs on me ;-)

---
I just mentioned the games (a full screen override redirect is not broken per 
se) to point out that this might not have the desired outcome.

---
[1] Do you btw. already have an opinion about reviving it?


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121299/#review71139
---


On Dez. 1, 2014, 6:49 nachm., Kai Uwe Broulik wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121299/
 ---
 
 (Updated Dez. 1, 2014, 6:49 nachm.)
 
 
 Review request for KDE Frameworks, kwin and Martin Gräßlin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can 
 be placed even ontop of active fullscreen windows in contrast to the 
 Notifications.
 
 Not sure whether a kde netwm thing is required or we could just use some 
 other method in KWin to decide placement.
 
 Also dunno what impact on ABI this has, I tried to only add enum values at 
 the end.
 
 
 Diffs
 -
 
   autotests/netwininfotestclient.cpp 16ba4b3 
   src/netwm.cpp 1ccba32 
   src/netwm_def.h 0352f33 
 
 Diff: https://git.reviewboard.kde.org/r/121299/diff/
 
 
 Testing
 ---
 
 In conjunction with Review 121300 these are now ontop of fullscreen windows.
 
 
 Thanks,
 
 Kai Uwe Broulik
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121299: Add NET::OSD window type

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121299/#review71139
---


Please notice that override redirects are above EVERY managed window, ie. if 
you fullscreen window happens to be an SDL(? some toolkit does this at least) 
game, the new layer will fail its job.

Random 3¢:
So either this is superfluous (the window should just be override redirect) or 
(reg. wayland and unmanaged clients) in NETWM (to get away from unmanaged 
clients in general)

Because that re-appeared over and over again lately:
I'd pesonally oppose this as but we always forget to make new QML stuff 
override redirect specific solution.

Otoh, if this actually was a canonical type w/ well defined behavior, we could 
make fine-grained adjustments, eg. have it not above keep-above fullscreens or 
similar.

- Thomas Lübking


On Nov. 30, 2014, 10:16 nachm., Kai Uwe Broulik wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121299/
 ---
 
 (Updated Nov. 30, 2014, 10:16 nachm.)
 
 
 Review request for KDE Frameworks, kwin and Martin Gräßlin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 This adds a NET::OSD window type for the OSD (eg. volume feedback) so it can 
 be placed even ontop of active fullscreen windows in contrast to the 
 Notifications.
 
 Not sure whether a kde netwm thing is required or we could just use some 
 other method in KWin to decide placement.
 
 Also dunno what impact on ABI this has, I tried to only add enum values at 
 the end.
 
 
 Diffs
 -
 
   autotests/netwininfotestclient.cpp 16ba4b3 
   src/netwm.cpp 1ccba32 
   src/netwm_def.h 0352f33 
 
 Diff: https://git.reviewboard.kde.org/r/121299/diff/
 
 
 Testing
 ---
 
 In conjunction with Review 121300 these are now ontop of fullscreen windows.
 
 
 Thanks,
 
 Kai Uwe Broulik
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118946: Adding property _GTK_SHOW_WINDOW_MENU to NET::Properties2

[Thomas Lübking]

 On Juni 26, 2014, 7:29 vorm., Thomas Lübking wrote:
  this is however not ABI relevant, so depending on schedules one *could* 
  wait for some specified NETWM hint (to not cruft the lib with the gtk+ prop 
  symbol: removing it from the enum is oc. not API stable, thus not possible) 
  and be introduced even with KWin 5.0.3 or whatever the final name scheme 
  will be ;-)
 
 Martin Gräßlin wrote:
 yeah I'm fine with delaying to post 5.0 in frameworks. Means KWin 5.0 
 will not have full support for it. But if it gets into KWindowSystem 5.1 we 
 can still add it in a minor KWin relase (and get killed by distros for 
 increasing requirement in a minor release :-P ).
 
 Thomas Lübking wrote:
 if this is causing downstream dep issues, maybe a generic 
 NETRootInfo::setSupported(const QString property) might be a good idea?
 
 Martin Gräßlin wrote:
 could be something to look into for 5.1, good idea.
 
 David Edmundson wrote:
 5.1 has been and gone. What's the status of this?

There hasn't even been a request from gtk+ devs to get this into NETWM.
I was about to rant that the GTK (gnome first) devs give a damn about 
interoperability and expect to run after us!, but google (wanted to be sure 
;-) pushed up
http://people.freedesktop.org/~cbrill/dri-log//dri-devel-2014-05-08.log

07:45 #wayland:  Jasper mgraesslin, whenever I send mails to wm-spec-list I 
don't get any replies, so I stopped bothering

Jasper is apparently the one who added support to mutter.

---

@Martin
I would suggest to either declare NETWM dead XOR formalize the board.

Eg. one would define a high council consisting of the maintainers of the 
relevant™ WMs (KWin, Mutter, Openbox and Enlightenment for sure. Compiz IFF 
Canonical promises to behave. Eventually awesome? I probably forgot an obvious 
one) an setup some rules.

a) A proposal of a high council member w/o veto of any other HC member within 
21 days is in.
b) A proposal of a non member that has seen no reaction from high council or 
delegates within 21 days causes reposts with increasing frequence.
c) New features MUST be announced as supported by the WM, ignoring that hint 
and just expecting it is a client bug by definition.
d) A proposal containing the term should or any equivalent is auto-invalid ;-)


I'd also suggest to move from wm-spec-l...@gnome.org to sth. like 
ne...@lists.freedesktop.org and/or have a fdo bugzilla bugtracker for handling 
concerns/proposals.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118946/#review60994
---


On Juni 26, 2014, 7:22 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118946/
 ---
 
 (Updated Juni 26, 2014, 7:22 vorm.)
 
 
 Review request for KDE Frameworks and kwin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 Adding property _GTK_SHOW_WINDOW_MENU to NET::Properties2
 
 Although non-standard it needs to be added to the NET::PRoperties2
 in order to have KWin announce it in the supported section.
 
 As soon as this gets standardized WM2ShowWindowMenu can be changed
 to point to both the proprietary GTK hint and the standardized hint.
 
 
 Diffs
 -
 
   autotests/netrootinfotestwm.cpp f8c28be51e5a5b19d436c54eede0e8659a65c84e 
   src/netwm.cpp 1daad1e5fc87fa85da6348a059d0ae0acec26eaf 
   src/netwm_def.h 0edadc085e08531ec81bcde5651e8475e8573091 
 
 Diff: https://git.reviewboard.kde.org/r/118946/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121197: KStartupInfo: use QX11Info::getTimestamp if appUserTime is 0

[Thomas Lübking]

 On Nov. 24, 2014, 2:01 nachm., Martin Gräßlin wrote:
  after checking 
  https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ : 
  this is a BC compatible change, but not SC. Seems like one needs to use a 
  different method name :-(

Where did you read this?
void foo() - void foo(int bar = 1);
isn't BC, but it is oc SC.

What you need to do is to overload explicitly:
void foo() - void foo(); void foo(int bar); // no default!


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121197/#review70849
---


On Nov. 24, 2014, 3:06 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121197/
 ---
 
 (Updated Nov. 24, 2014, 3:06 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 337798
 https://bugs.kde.org/show_bug.cgi?id=337798
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 KStartupInfo::createNewStartupId is supposed to return a new
 id with a properly setup user timestamp. But if QX11Info::appUserTime
 returns 0 this was included in the id and cannot be considered a
 properly setup user timestamp. An app user time of 0 is the case
 for klauncher. If an application uses this timestamp of 0 passed from
 the startup notification it causes problems with passing focus to it
 from KWin. The application sets the timestamp in the
 _NET_WM_USER_TIME property and the value 0 has the special meaning
 of requesting to not be initially mapped. KWin honors that and doesn't
 focus the window. An application is not supposed to interpret a 0 time
 stamp passed through the startup notification as a special value to get
 the current time. It is totally fine to expect that it gets a proper
 value passed. Thus one cannot blame applications for not interpreting
 this value accordingly. An example for an application passing the 0
 to the _NET_WM_USER_TIME is Firefox. Starting Firefox in a Plasma 5
 session through e.g. the launcher results in KWin not passing focus
 to it and instead demands attention. This is clearly wrong and not
 intended.
 
 The solution in this change is to get the current timestamp from
 the X server in case the appUserTime is 0. This fixes the described
 problem with Firefox: it now gets properly focused on starting
 through a launcher.
 
 
 Diffs
 -
 
   autotests/kstartupinfo_unittest.cpp 
 f4b607d2a21da7dcf32434c00b2bdeeaf401ee65 
   src/kstartupinfo.h ff4454b4102ecaea7067adb8c3a56d7f500250e0 
   src/kstartupinfo.cpp 03418fce1a154ea388d0f65665dc0c9724a5623a 
 
 Diff: https://git.reviewboard.kde.org/r/121197/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121197: KStartupInfo: use QX11Info::getTimestamp if appUserTime is 0

[Thomas Lübking]

 On Nov. 24, 2014, 2:01 nachm., Martin Gräßlin wrote:
  after checking 
  https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ : 
  this is a BC compatible change, but not SC. Seems like one needs to use a 
  different method name :-(
 
 Thomas Lübking wrote:
 Where did you read this?
 void foo() - void foo(int bar = 1);
 isn't BC, but it is oc SC.
 
 What you need to do is to overload explicitly:
 void foo() - void foo(); void foo(int bar); // no default!

 The solution in this change is to get the current timestamp from the X server 
 in case the appUserTime is 0.

This is (luckily) not what the patch does atm. (I took the default w/ a grain 
of salt, as the proposed behavior would have prevented to explicitly start the 
application w/o taking the focus)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121197/#review70849
---


On Nov. 24, 2014, 3:06 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121197/
 ---
 
 (Updated Nov. 24, 2014, 3:06 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 337798
 https://bugs.kde.org/show_bug.cgi?id=337798
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 KStartupInfo::createNewStartupId is supposed to return a new
 id with a properly setup user timestamp. But if QX11Info::appUserTime
 returns 0 this was included in the id and cannot be considered a
 properly setup user timestamp. An app user time of 0 is the case
 for klauncher. If an application uses this timestamp of 0 passed from
 the startup notification it causes problems with passing focus to it
 from KWin. The application sets the timestamp in the
 _NET_WM_USER_TIME property and the value 0 has the special meaning
 of requesting to not be initially mapped. KWin honors that and doesn't
 focus the window. An application is not supposed to interpret a 0 time
 stamp passed through the startup notification as a special value to get
 the current time. It is totally fine to expect that it gets a proper
 value passed. Thus one cannot blame applications for not interpreting
 this value accordingly. An example for an application passing the 0
 to the _NET_WM_USER_TIME is Firefox. Starting Firefox in a Plasma 5
 session through e.g. the launcher results in KWin not passing focus
 to it and instead demands attention. This is clearly wrong and not
 intended.
 
 The solution in this change is to get the current timestamp from
 the X server in case the appUserTime is 0. This fixes the described
 problem with Firefox: it now gets properly focused on starting
 through a launcher.
 
 
 Diffs
 -
 
   autotests/kstartupinfo_unittest.cpp 
 f4b607d2a21da7dcf32434c00b2bdeeaf401ee65 
   src/kstartupinfo.h ff4454b4102ecaea7067adb8c3a56d7f500250e0 
   src/kstartupinfo.cpp 03418fce1a154ea388d0f65665dc0c9724a5623a 
 
 Diff: https://git.reviewboard.kde.org/r/121197/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121197: KStartupInfo: use QX11Info::getTimestamp if appUserTime is 0

[Thomas Lübking]

 On Nov. 24, 2014, 2:01 nachm., Martin Gräßlin wrote:
  after checking 
  https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ : 
  this is a BC compatible change, but not SC. Seems like one needs to use a 
  different method name :-(
 
 Thomas Lübking wrote:
 Where did you read this?
 void foo() - void foo(int bar = 1);
 isn't BC, but it is oc SC.
 
 What you need to do is to overload explicitly:
 void foo() - void foo(); void foo(int bar); // no default!
 
 Thomas Lübking wrote:
  The solution in this change is to get the current timestamp from the X 
 server in case the appUserTime is 0.
 
 This is (luckily) not what the patch does atm. (I took the default w/ a 
 grain of salt, as the proposed behavior would have prevented to explicitly 
 start the application w/o taking the focus)
 
 Martin Gräßlin wrote:
 yoy cannot add an overload (BC, but not SC: makes func ambiguous), 
 adding overloads to already overloaded functions is ok (any use of func 
 already needed a cast).
 
 to your other comment: rbt failed updating the comment.

Ah - fucktors, e... functors ;-)
Yes, indeed :-(


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121197/#review70849
---


On Nov. 24, 2014, 3:06 nachm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121197/
 ---
 
 (Updated Nov. 24, 2014, 3:06 nachm.)
 
 
 Review request for KDE Frameworks.
 
 
 Bugs: 337798
 https://bugs.kde.org/show_bug.cgi?id=337798
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 KStartupInfo::createNewStartupId is supposed to return a new
 id with a properly setup user timestamp. But if QX11Info::appUserTime
 returns 0 this was included in the id and cannot be considered a
 properly setup user timestamp. An app user time of 0 is the case
 for klauncher. If an application uses this timestamp of 0 passed from
 the startup notification it causes problems with passing focus to it
 from KWin. The application sets the timestamp in the
 _NET_WM_USER_TIME property and the value 0 has the special meaning
 of requesting to not be initially mapped. KWin honors that and doesn't
 focus the window. An application is not supposed to interpret a 0 time
 stamp passed through the startup notification as a special value to get
 the current time. It is totally fine to expect that it gets a proper
 value passed. Thus one cannot blame applications for not interpreting
 this value accordingly. An example for an application passing the 0
 to the _NET_WM_USER_TIME is Firefox. Starting Firefox in a Plasma 5
 session through e.g. the launcher results in KWin not passing focus
 to it and instead demands attention. This is clearly wrong and not
 intended.
 
 The solution in this change is to get the current timestamp from
 the X server in case the appUserTime is 0. This fixes the described
 problem with Firefox: it now gets properly focused on starting
 through a launcher.
 
 
 Diffs
 -
 
   autotests/kstartupinfo_unittest.cpp 
 f4b607d2a21da7dcf32434c00b2bdeeaf401ee65 
   src/kstartupinfo.h ff4454b4102ecaea7067adb8c3a56d7f500250e0 
   src/kstartupinfo.cpp 03418fce1a154ea388d0f65665dc0c9724a5623a 
 
 Diff: https://git.reviewboard.kde.org/r/121197/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121197: KStartupInfo: use QX11Info::getTimestamp if appUserTime is 0

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121197/#review70739
---



src/kstartupinfo.cpp
https://git.reviewboard.kde.org/r/121197/#comment49475

Should this not rather be ::getTimestamp() unconditionally?

This is supposed to be a hint when the application was triggered, not when 
there was the last interaction in the calling client.

::appUserTime() might be any random value in the past, thus pollute the FSP 
heuristics.


- Thomas Lübking


On Nov. 21, 2014, 10:44 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121197/
 ---
 
 (Updated Nov. 21, 2014, 10:44 vorm.)
 
 
 Review request for KDE Frameworks, kwin and Plasma.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 KStartupInfo::createNewStartupId is supposed to return a new
 id with a properly setup user timestamp. But if QX11Info::appUserTime
 returns 0 this was included in the id and cannot be considered a
 properly setup user timestamp. An app user time of 0 is the case
 for klauncher. If an application uses this timestamp of 0 passed from
 the startup notification it causes problems with passing focus to it
 from KWin. The application sets the timestamp in the
 _NET_WM_USER_TIME property and the value 0 has the special meaning
 of requesting to not be initially mapped. KWin honors that and doesn't
 focus the window. An application is not supposed to interpret a 0 time
 stamp passed through the startup notification as a special value to get
 the current time. It is totally fine to expect that it gets a proper
 value passed. Thus one cannot blame applications for not interpreting
 this value accordingly. An example for an application passing the 0
 to the _NET_WM_USER_TIME is Firefox. Starting Firefox in a Plasma 5
 session through e.g. the launcher results in KWin not passing focus
 to it and instead demands attention. This is clearly wrong and not
 intended.
 
 The solution in this change is to get the current timestamp from
 the X server in case the appUserTime is 0. This fixes the described
 problem with Firefox: it now gets properly focused on starting
 through a launcher.
 
 
 Diffs
 -
 
   autotests/kstartupinfo_unittest.cpp 
 f4b607d2a21da7dcf32434c00b2bdeeaf401ee65 
   src/kstartupinfo.cpp 03418fce1a154ea388d0f65665dc0c9724a5623a 
 
 Diff: https://git.reviewboard.kde.org/r/121197/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121198: Drop incorrect warnings when using KXMessages without QX11Info

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121198/#review70740
---

Ship it!


Ship It!

- Thomas Lübking


On Nov. 21, 2014, 11:04 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/121198/
 ---
 
 (Updated Nov. 21, 2014, 11:04 vorm.)
 
 
 Review request for KDE Frameworks, kwin and Plasma.
 
 
 Bugs: 340310
 https://bugs.kde.org/show_bug.cgi?id=340310
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 The idea was to not perform the action if QX11Info reports that its
 not the xcb plugin. But this check is not correct as those methods
 are not going through QX11Info at all. In addition they pass in either
 an xcb connection or XLib display which is totally fine and needed
 for example if we want Wayland applications interact with the X11
 world. Also it causes problems for e.g. kdeinit which is not a
 Q*Application and thus does not have a QX11Info.
 
 At the same time fix an incorrect usage of QX11Info::connection
 where an xcb_connection_t* is already passed in to the method.
 
 BUG: 340310
 
 
 Diffs
 -
 
   src/kxmessages.cpp 2e625d29c4fd4a5056d792f565d53dea5e6529fd 
 
 Diff: https://git.reviewboard.kde.org/r/121198/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 121020: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121020/
---

Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: plasma-desktop


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285t=123587
https://git.reviewboard.kde.org/r/121019/
https://git.reviewboard.kde.org/r/121021/


Diffs
-

  kcms/krdb/krdb.cpp 8452aa5 
  kcms/style/kcmstyle.cpp 9a13f45 

Diff: https://git.reviewboard.kde.org/r/121020/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 121021: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121021/
---

Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: kdelibs4support


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285t=123587
https://git.reviewboard.kde.org/r/121019/
https://git.reviewboard.kde.org/r/121020/


Diffs
-

  src/kdeui/k4style.cpp a1a2ab1 
  src/kdeui/kglobalsettings.h d63ac69 

Diff: https://git.reviewboard.kde.org/r/121021/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 121019: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121019/
---

Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: frameworkintegration


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285t=123587
https://git.reviewboard.kde.org/r/121020/
https://git.reviewboard.kde.org/r/121021/


Diffs
-

  autotests/kdeplatformtheme_changed_kdeglobals 910e0e3 
  autotests/kdeplatformtheme_kdeglobals df52410 
  src/kstyle/kstyle.cpp b5f7363 
  src/platformtheme/khintssettings.cpp 8799216 

Diff: https://git.reviewboard.kde.org/r/121019/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120257: Add support for initial mapping state of WM_HINTS

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120257/#review66844
---

Ship it!



src/netwm_def.h
https://git.reviewboard.kde.org/r/120257/#comment46621

soon we're gonna need NET::Properties3 ;-)


- Thomas Lübking


On Sept. 18, 2014, 8 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120257/
 ---
 
 (Updated Sept. 18, 2014, 8 vorm.)
 
 
 Review request for KDE Frameworks and kwin.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 To complement the other flags already taken from WM_HINTS, we also
 read the initial mapping state.
 
 
 Diffs
 -
 
   autotests/netwininfotestclient.cpp 2795cbf11277d6dc78d24979797e6d6c9cd3750e 
   src/netwm.h 0535c39efc3d4fc428545abd599ce927fbf8903d 
   src/netwm.cpp 3107a2388c2fb3f1a8ad97e466be40e6977e064b 
   src/netwm_def.h c8b1ba0a99a5f1e8a4939d0304b0facde25c59a8 
   src/netwm_p.h 3bb8213acd39f86c8c1c49015243d2bd3e82a8ba 
 
 Diff: https://git.reviewboard.kde.org/r/120257/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120213: Add support for protocols to NETWinInfo

[Thomas Lübking]

 On Sept. 15, 2014, 8:34 nachm., Thomas Lübking wrote:
  src/netwm.cpp, line 4630
  https://git.reviewboard.kde.org/r/120213/diff/1/?file=312330#file312330line4630
 
  Tarzan speech. Consider:
  - TakeFocusProtocol
  - Protocol::TakeFocus
 
 Martin Gräßlin wrote:
 we are (to my understanding) not allowed to use the enum class from 
 C++11. As netwm_def.h is shared between the implementations I cannot even 
 argument with but it's not compiled on Windows. So I'll switch to the 
 TakeFocusProtocol approach.

You could use a nested namespace, but i'm absolutely fine with 
TakeFocusProtocol.


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120213/#review66605
---


On Sept. 16, 2014, 5:55 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120213/
 ---
 
 (Updated Sept. 16, 2014, 5:55 vorm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 NETWinInfo is now able to read all known protocols set on WM_PROTOCOLS
 property and provides the set protocols as flags.
 
 So far the following protocols are supported:
 * WM_TAKE_FOCUS (ICCCM 4.1.2.7)
 * WM_DELETE_WINDOW (ICCCM 4.1.2.7)
 * _NET_WM_SYNC_REQUEST (EWMH)
 * _NET_WM_PING (EWMH)
 * _NET_WM_CONTEXT_HELP (KDE specific extension to EWMH)
 
 CHANGELOG: NETWinInfo provides convenient wrapper for WM_PROTOCOLS.
 
 
 Diffs
 -
 
   autotests/netwininfotestclient.cpp 7d941f4b32a546c8284ab81995f594cd9e69617e 
   src/netwm.h 436ad2fd449b48a36d3bb15754f7c8c64f52c8dd 
   src/netwm.cpp 1431cf6bae605050e2e86f7ca60357ddb7736d32 
   src/netwm_def.h 12abfff03af5b81a60ec03a5366b6ff0b95d35d0 
   src/netwm_p.h a586450211846af464c9014f586e14b0e808cd75 
 
 Diff: https://git.reviewboard.kde.org/r/120213/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120213: Add support for protocols to NETWinInfo

[Thomas Lübking]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120213/#review66633
---

Ship it!


Ship It!

- Thomas Lübking


On Sept. 16, 2014, 5:55 vorm., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/120213/
 ---
 
 (Updated Sept. 16, 2014, 5:55 vorm.)
 
 
 Review request for KDE Frameworks.
 
 
 Repository: kwindowsystem
 
 
 Description
 ---
 
 NETWinInfo is now able to read all known protocols set on WM_PROTOCOLS
 property and provides the set protocols as flags.
 
 So far the following protocols are supported:
 * WM_TAKE_FOCUS (ICCCM 4.1.2.7)
 * WM_DELETE_WINDOW (ICCCM 4.1.2.7)
 * _NET_WM_SYNC_REQUEST (EWMH)
 * _NET_WM_PING (EWMH)
 * _NET_WM_CONTEXT_HELP (KDE specific extension to EWMH)
 
 CHANGELOG: NETWinInfo provides convenient wrapper for WM_PROTOCOLS.
 
 
 Diffs
 -
 
   autotests/netwininfotestclient.cpp 7d941f4b32a546c8284ab81995f594cd9e69617e 
   src/netwm.h 436ad2fd449b48a36d3bb15754f7c8c64f52c8dd 
   src/netwm.cpp 1431cf6bae605050e2e86f7ca60357ddb7736d32 
   src/netwm_def.h 12abfff03af5b81a60ec03a5366b6ff0b95d35d0 
   src/netwm_p.h a586450211846af464c9014f586e14b0e808cd75 
 
 Diff: https://git.reviewboard.kde.org/r/120213/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel