KDE Custom Shortcuts broken, problem's id'd as in khotkeys

[Thomas Lübking]

This is https://bugreports.qt.io/browse/QTBUG-48795
Qt5 builds it's internal events as combination of (synthetic) keys and HW
modifiers.

That is *clearly* a fucking bug in Qt: either it ignores synthetic
events (a regression compared to Qt4) or not, but just creating random
events out of different sources should certainly not be intended.

Cheers,
Thomas

PS: Sorry for pm and thread breaking;
I simply ran away from Qt (and unsubscribed from KDE MLs) because it has
meanwhile turned into an unusable PoS reg. the desktop.
And I believe that *is* intentional to support their desperation to
replicate the JavaFX script failure.

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: Error buildin KDE (KF5)

[Thomas Lübking]

At least the QtCore and KF5/KGlobalAccel include paths are not added (or
mislocated)
The key is again the cmake log.

On a sidenote: your cmake is terribly outdated. It's listed as minimum
for kf5, but even that was wrong (see before issue) and I would not bet
even your right arm that it's sufficient for all other repos (despite
they may lazily list even older versions as minimum ;-)

Cheers,
Thomas

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: KDE file dialog

[Thomas Lübking]

On Sonntag, 6. März 2016 11:34:32 CEST, Martin Graesslin wrote:

I don't know how often we have heard over the last decade that 
Plasma's look 
and feel is terribly because of fonts.


I'd have assumed that'd be because of the freetype rasterizers being, errr 
ummm... shit (almost as shit as cleartype ;-) - and be resolved by the CFF 
rasterizer?

Cheers,
Thomas

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: KConfig - setStandardButtons() breaks Help button?

[Thomas Lübking]

On Samstag, 5. März 2016 08:34:27 CEST, Frederik Schwarzer wrote:

Since in my use case, some of the values in the dialog are calculated 
and (to my understanding) cannot be handled correctly by Kcfg, I want 
to get rif of the "Restore Defaults" button. So I add this line:


 dialog->setStandardButtons(QDialogButtonBox::Ok |
 QDialogButtonBox::Cancel | QDialogButtonBox::Help);

but then the Help button does not open the Help browser anymore.



::setStandardButtons simply forwards the call to the internal button box which 
nukes all button objects (incl. their slot connections) and recreates them w/ 
the new item list.
The most straight forward solution is likely to hide or delete 
button(QDialogButtonBox::RestoreDefaults)

Otherwise you'll have to grep through to the buttonbox and remove it 
"correctly" or rebind the help button to the showHelp() slot.
It though looks like you'd destroy more bindings by resetting the 
standardButtons (which you all need to restore)


I'm not sure the HIG team would like to remove the defaults button, though.
:-P

Rather replace the restore defaults button, resp. re-link it to your own 
calculation slot.


Cheers,
Thomas

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: KDE file dialog

[Thomas Lübking]

On Mittwoch, 2. März 2016 12:51:28 CEST, Martin Graesslin wrote:

On Wednesday, March 2, 2016 12:09:17 PM CET Sven Brauch wrote:

On 03/02/2016 11:56 AM, Thomas Lübking wrote: ...


Just saying: I wrote that SNI integration and I have never heard of that 
problem before Sven mentioned it here.


The client deadlocked flooding the dbus. Only happened if the SNI daemon was/is 
unreachable.
See bug #350288 and linked dupe.

Iirc it was not the SNI daemon which crashed, but some other ded module (taking 
kded with it, causing this as follow-up and made me wonder what would happen if 
someone simply unticked that daemon => unusable system)

Cheers,
Thomas

Re: KDE file dialog

[Thomas Lübking]

On Mittwoch, 2. März 2016 11:19:42 CEST, Martin Graesslin wrote:

On Wednesday, March 2, 2016 11:08:30 AM CET Mark Gaiser wrote:
On Wed, Mar 2, 2016 at 9:42 AM, Martin Graesslin 
 wrote: ...


What is that "kf5" plugin you talk of?


The origin of this thread was that the default Qt file dialog sucks, so there's 
desire for the KIO KFileWidget (but kio is already tier3 ??)

The KDE QPA plugin however has some trouble on non-plasma environment 
(apparently notably on the Systray/SNI part), so it cannot be used outside 
plasmashell.

I *assume* that the core problem is that the plasma QPA isn't sufficiently 
robust, eg. if the SNI daemon doesn't come up (I once had it crashing here what 
drove me into the mentioned FD exception through mindless dbus calls - not 
funny) it should probably give up and resort to letting through the unhooked 
FDO systray.

Imo that's a more issue: IPC is I/O, ie. unreliable. You cannot provide 
functionality that relies on working IPC, but hard-relying on it is bad design 
(nb. that the failing kded module make _every_ Qt client using QSystemTray 
unusable and trying to knock out your system)

=> fix the QPA to handle I/O (dbus etc.) more robust and everyone's happy?


Cheers,
Thomas

---

If anyone would like to discuss whether pushing SNI or which systray protocol 
should be used or is better: Spare yor time.
I firmly believe that the systray is a completely braindead concept, no matter 
how it's done. You wanted to go for a backend/frontend implementation itfp.

Re: KDE file dialog

[Thomas Lübking]

On Dienstag, 1. März 2016 19:42:58 CEST, Sven Brauch wrote:


Otherwise I agree with your reasoning. I'm just not sure what we can
effectively do about it.


Build option to build the kde/plasma QPA into a strict "KF5" (tier 1) QPA?
Forking sucks terribly and a tier 1 QPA might be interesting for eg. lxqt users 
(drawing them into KF5)


Having the plugin in an extra repository imo
doesn't help much, and I really don't mind having plasma installed.


The repo is probably not that much of an issue.
Most users would get it from their distro and that could build two QPAs and 
ship the KF5 variant extra. Only exception are devs and gentoo users and it's 
not *that* much disk space.

Alternative solution would be git submodules, but that looks like quite some 
work (notably since you'd want the kf5 version that you still need to find some 
repo for to default off while the plasma version defaults on)

Cheers,
Thomas

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: mailing list archive gone ?

[Thomas Lübking]

On Samstag, 27. Februar 2016 16:09:00 CEST, Martin Koller wrote:

Hi,

https://mail.kde.org/pipermail/kde-core-devel/

shows only 3 mails from the year 2000.
Where are the other archives gone ?


No idea, but it's still on https://marc.info/?l=kde-core-devel=1=2

Cheers,
Thomas

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 127102: Use fixed width for digital clock applet

[Thomas Lübking]

> On Feb. 18, 2016, 12:05 a.m., David Edmundson wrote:
> > applets/digital-clock/package/contents/ui/DigitalClock.qml, line 555
> > <https://git.reviewboard.kde.org/r/127102/diff/1/?file=444552#file444552line555>
> >
> > rather than looping, can we use FontMetric's maximumCharacterWidth
> > 
> >  * numChars.
> > 
> > Then we could kill sizeHelper competely (FontMetric's didn't exist when 
> > this was written)
> 
> Marco Martin wrote:
> hoping maximumCharacterWidth is reliable for all fonts, this loop really 
> needs to go
> 
> Daniel Faust wrote:
> As far as I understand maximumCharacterWidth returns the width of the 
> widest character of the font - which can be ridiculously wide given that the 
> font supports some wild unicode characters.
> I did a quick test and maximumCharacterWidth returned about twice the 
> width actually needed.
> 
> Martin Klapetek wrote:
> You still don't need this whole loop though, just find the biggest in 0-9 
> and use that for all the numbers (except year).
> 
> Daniel Faust wrote:
> Since I don't know the time format in advance, I have to construct 
> different times and process them with Qt.formatTime.
> That means, I have to test 0-9 for hours/minutes/seconds, 0-2 for tens of 
> hours and 0-5 for tens of minutes/seconds. Otherwise the constructed times 
> will be invalid.
> This can be done with three loops and it would bring down the amount of 
> advanceWidth and formatTime calls from 24+60=84 to 3+6+10=19.
> 
> Martin Klapetek wrote:
> You don't actually need them as a time, you just need them as a 
> placeholder. At which point it doesn't matter if it's a valid time or not. 
> You can even format 12:12 with Qt.formatTime and then replace the numbers 
> with your placeholder number.
> 
> In other words, the time format /is/ known in advance.
> 
> Daniel Faust wrote:
> I'm not sure if I understand you, I just tried it with a single loop from 
> 0-9 to construct the time strings:
> 
> for (var i=0; i<=9; i++) {
>   var str = Qt.formatTime(new Date(2000, 0, 1, i*10 + i, i*10 + i, i*10 + 
> i), main.timeFormat);
>   console.log("hour: " + (i*10 + i) + ", minute: " + (i*10 + i) + " -> " 
> + str);
> }
> 
> qml: hour: 0, minute: 0 -> 00:00
> qml: hour: 11, minute: 11 -> 11:11
> qml: hour: 22, minute: 22 -> 22:22
> qml: hour: 33, minute: 33 -> 09:33
> qml: hour: 44, minute: 44 -> 20:44
> qml: hour: 55, minute: 55 -> 07:55
> qml: hour: 66, minute: 66 -> 19:07
> qml: hour: 77, minute: 77 -> 06:18
> qml: hour: 88, minute: 88 -> 17:29
> qml: hour: 99, minute: 99 -> 04:40
> 
> Thomas Lübking wrote:
> Forget about the timeFormat - you iterate from 0-9 and figure the widest 
> glyphs in [0,1], [0,5] and [0,9] - let's reasonably say "0,3 and 7".
> Then you check the width for 00:37
> 
> (If you also need the date, you also need to check for the widest glyph 
> in [0,3])
> 
> Daniel Faust wrote:
> That would work if it wasn't for 12 hour time formats.
> For 24 hour formats you actually need [0,2], [0,3] and [0,9] for the hour 
> and [0,5] and [0,9] for the minutes/seconds.
> For 12 hour formats AM you need [0,1], [0,2] and [0,9] for the hour and 
> [0,5] and [0,9] for the minutes/seconds.
> For 12 hour formats PM you need [1,2], [3,9] and [0,3] for the hour and 
> [0,5] and [0,9] for the minutes/seconds.
> On top of that you need to account for format changes like hour=0, 
> minute=0 -> "12:00 AM".
> 
> The code for this logic would be hard to maintain.
> 
> Daniel Faust wrote:
> Ok, what about this?
> Find the widest glyph between 0 and 9.
> Use a regex to replace [hmsz] from the format string with the widest 
> number. (This will produce strings like "44:44")
> Use Qt.formatTime once with an AM time and once with a PM time and choose 
> the widest of the produced strings.
> This code is fairly short and overestimates the total width only slightly.

As long as the formatter accepts invalid times, that's certainly the most 
straight forward solution, yes.


- Thomas


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


On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply

Re: Review Request 127102: Use fixed width for digital clock applet

[Thomas Lübking]

> On Feb. 18, 2016, 12:05 a.m., David Edmundson wrote:
> > applets/digital-clock/package/contents/ui/DigitalClock.qml, line 555
> > 
> >
> > rather than looping, can we use FontMetric's maximumCharacterWidth
> > 
> >  * numChars.
> > 
> > Then we could kill sizeHelper competely (FontMetric's didn't exist when 
> > this was written)
> 
> Marco Martin wrote:
> hoping maximumCharacterWidth is reliable for all fonts, this loop really 
> needs to go
> 
> Daniel Faust wrote:
> As far as I understand maximumCharacterWidth returns the width of the 
> widest character of the font - which can be ridiculously wide given that the 
> font supports some wild unicode characters.
> I did a quick test and maximumCharacterWidth returned about twice the 
> width actually needed.
> 
> Martin Klapetek wrote:
> You still don't need this whole loop though, just find the biggest in 0-9 
> and use that for all the numbers (except year).
> 
> Daniel Faust wrote:
> Since I don't know the time format in advance, I have to construct 
> different times and process them with Qt.formatTime.
> That means, I have to test 0-9 for hours/minutes/seconds, 0-2 for tens of 
> hours and 0-5 for tens of minutes/seconds. Otherwise the constructed times 
> will be invalid.
> This can be done with three loops and it would bring down the amount of 
> advanceWidth and formatTime calls from 24+60=84 to 3+6+10=19.
> 
> Martin Klapetek wrote:
> You don't actually need them as a time, you just need them as a 
> placeholder. At which point it doesn't matter if it's a valid time or not. 
> You can even format 12:12 with Qt.formatTime and then replace the numbers 
> with your placeholder number.
> 
> In other words, the time format /is/ known in advance.
> 
> Daniel Faust wrote:
> I'm not sure if I understand you, I just tried it with a single loop from 
> 0-9 to construct the time strings:
> 
> for (var i=0; i<=9; i++) {
>   var str = Qt.formatTime(new Date(2000, 0, 1, i*10 + i, i*10 + i, i*10 + 
> i), main.timeFormat);
>   console.log("hour: " + (i*10 + i) + ", minute: " + (i*10 + i) + " -> " 
> + str);
> }
> 
> qml: hour: 0, minute: 0 -> 00:00
> qml: hour: 11, minute: 11 -> 11:11
> qml: hour: 22, minute: 22 -> 22:22
> qml: hour: 33, minute: 33 -> 09:33
> qml: hour: 44, minute: 44 -> 20:44
> qml: hour: 55, minute: 55 -> 07:55
> qml: hour: 66, minute: 66 -> 19:07
> qml: hour: 77, minute: 77 -> 06:18
> qml: hour: 88, minute: 88 -> 17:29
> qml: hour: 99, minute: 99 -> 04:40

Forget about the timeFormat - you iterate from 0-9 and figure the widest glyphs 
in [0,1], [0,5] and [0,9] - let's reasonably say "0,3 and 7".
Then you check the width for 00:37

(If you also need the date, you also need to check for the widest glyph in 
[0,3])


- Thomas


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


On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127102/
> ---
> 
> (Updated Feb. 17, 2016, 4:23 p.m.)
> 
> 
> Review request for kde-workspace and Plasma.
> 
> 
> Bugs: 347724
> https://bugs.kde.org/show_bug.cgi?id=347724
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> ---
> 
> Currently the width of the date label is not fixed but changes depending on 
> the text. This causes the entire applet to change its width (if the time is 
> the widest displayed item). This in turn can cause all other applets in the 
> same panel to move whenever the displayed time changes.
> 
> This patch uses FontMetrics to iterate over all possible time strings (with 
> different width) and chooses the widest of them as reference for the fixed 
> width of the time label.
> 
> This way the width of the applet stays the same (unless the date is displayed 
> and changes). The text remains centered though, which means that it can still 
> move within the applet when the time changes.
> 
> 
> Diffs
> -
> 
>   applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071 
> 
> Diff: https://git.reviewboard.kde.org/r/127102/diff/
> 
> 
> Testing
> ---
> 
> Works with horizontal and vertical panel.
> Also displaying different combinations of "seconds", "date" and "timezone" 
> works.
> 
> 
> Thanks,
> 
> Daniel Faust
> 
>

Re: License of the Breeze style

[Thomas Lübking]

On Mittwoch, 17. Februar 2016 07:58:46 CEST, Martin Graesslin wrote:

On Tuesday, February 16, 2016 8:15:57 PM CET Александр Волков wrote:



I've noticed that the Breeze style is released under GPL-2+ license:
Why not LGPL? It's a library after all.


The style is not a library, it's a plugin. As it's released together with 
Plasma the license choice of GPL looks quite correct to me.


According to the FSF cheat sheet [1] that's actually a "problem", because, 
while LGPL is an inappropriate license in any case, the GPL forbids [2] to load the GPL 
plugin in a non-GPL compatibly licensed application, ie. strictly spoken, any application 
using the non-free Qt license MUST NOT use the Breeze style (this becomes even more 
interesting reg. the QPA plugin - notably as it's the user who loads the plugin ;-)

I recall some trouble with Debian in this regard, because they wanted to ship 
Baghira but were uncomfortable with the BSD Style license it inherited from 
Mosfet's Liquid. I never thought too much about the issue and just constrained 
the license, but it seems BSD was chosen deliberately back then?

I guess GPL is fine and if users use non-free Qt clients on the Plasma QPA, we 
just look the other side?

Cheers,
Thomas


[1] http://www.gnu.org/licenses/gpl-faq.en.html#GPLAndPlugins
[2] http://www.gnu.org/licenses/gpl-faq.en.html#GPLPluginsInNF

Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut

[Thomas Lübking]

* degrades user trust. Imagine if every 
> application did this - would you use a system which kept popping up with info 
> boxes instead of doing the work? Especially when you *need* the system to 
> take care of a problem and the system tells you "I can't do this, try doing 
> this" - I can think of no better way to shatter a users trust; one thing is 
> already broken, we should not make it feel *more* broken.
> 
> Finally, xkill is a separate utility entirely, I question if it should be 
> advertised at all in ksysguard. One is a UI-oriented solution, one is a 
> shortcut solution. Should Kate advertise KDevelop features? Should Juk 
> advertise Amarok? Should the volume control widget advertise the Pulse 
> command line utilities? Just because they do related things doesn't 
> necessarily mean they should advertise each-other.
> 
> Anyway, I would propose one of these alternative solutions: 
>  - Having a tip at the bottom of the window between the task list and 
> status bar which goes: "(i) You can force close a specific window by pressing 
> #shortcut#". This tip could be closed/hidden permanently by the user.
>  - Moving the information to the "help" menu. Help > "How to kill a 
> window". A little less discoverable, but much more appropriate. 
>  - Have it actually perform the function. The option will launch an 
> [OK|Cancel] dialog with "You will force-close the next window you click on. 
> Hit ESC to cancel at any time". When the user presses [OK] it properly 
> executes the xkill utility. This is more future-proof in case Kwin offers a 
> nicer KDE/Plasma-centric with the warnings and labels built-in, because we 
> could just adapt this in the future to use that.
>  - Clean out the tooltip. Remove the parts on right-clicking and what-if. 
> Users will visit help if they need it, and right-clicking is common 
> functionality. Instead of adding mroe to make thing visible, we remove noise.
>  - Leave it as-is. *This is an option.* There are lots of places we could 
> be tempted to tell users about our functionality, but the fact is there will 
> always be things a computer can do that users don't know about.
> 
> As far as code complexity and keeping it less complex, I personally 
> beleive that if we're introducing code for *anything* it should at least be 
> functional. To me increasing code complexity for a feature is acceptible, but 
> incrementing the complexity even a little for half-measures isn't really 
> justifiable.
> 
> Thomas Lübking wrote:
> On the bottom line, this is the "KWin has no real GUI" problem.
> 
> Aside the xkill (we're more talking about the feature built into KWin 
> than the x util here) there're several other kwin features only available 
> through shortcuts (eg. invert screen colors, a bunch of effects, random 
> scripts) and not even all window related actions (packing...) are somehow 
> announced in some GUI (too much pro stuff)
> 
> Maybe the question is whether KWin needs an entry in eg. the systray 
> where it can show a window which exposes those features - including their 
> assigned shortcuts or - as alternative - a "global" section in the Alt+F3 
> menu?
> 
> Gregor Mi wrote:
> Hi Ken,
> 
> I go through your feedback paragraph by paragraph. I try to keep it short 
> so it might seem a bit abrupt.
> 
> > Why do we need to go so far out of our way to advertise XKill when we 
> are capable of killing apps from the monitor?
> 
> Because it would help users to find the feature. For me, it is one of 
> those features that I don't look for if I don't know it exists. The system 
> monitor which is able to kill processes is the most suitable place to 
> advertise this feature.
> 
> > Why would someone who won't stop for a tooltip know to press the "V" 
> button?
> 
> The tooltip should be used "where the function isn't clear from the 
> label" (said Thomas Pfeiffer above). This implies that when the user thinks 
> that the meaning of the button is clear he probably won't read the tooltip.
> 
> > I also dislike that it advertised as a feature and not a help option, 
> and users with a crashing/frozen application will only be more frusterated if 
> they think they have a solution - and are instead given a manual
> 
> I disagree: if I as a user am in a situation where I need help, I am glad 
> I get some, and be it a pointer to some manual. It is better than nothing. 
> For example gparted does this when you are about to move your systems hard 
> drive: it shows a message box with a short explanation and a lin

Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut

[Thomas Lübking]

> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote:
> > > If someone has changed the shortcut, they should know what shortcut they 
> > > set it to, right? So having the tooltip just say "To kill a specific 
> > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" 
> > > should do the trick, right?
> > 
> > In principle, I agree with you here. :) But...
> > 
> > I have these premises in mind:
> > 
> > 1. There might be a situation where the user can't remember what he has 
> > done.
> > 2. I prefer precise over approximate information if it is reasonable easy 
> > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now 
> > easily possible and the patch is already written)
> > 3. Somewhere I read that tooltips are to be avoided where possible => this 
> > patch factors some information out of a tooltip which in itself is 
> > desirable.
> > 4. For the user, the discoverability of the feature after applying this 
> > patch is better than before (though not perfect, sure).
> 
> Thomas Pfeiffer wrote:
> 2. If we can get the current shortcut, why can't we get it for the 
> tooltip?
> 3. Wait, are we talking about different tooltips? I do _not_ mean the one 
> you get when clicking the ? icon in the window decoration. Those should be 
> avoided because few people even notice that button. What I mean is the thing 
> that pops up when hovering over a control. Those are strongly encouraged in 
> the HIG, in fact they should be used on every control where the function 
> isn't clear from the label.
> 4. Clicking a button just to get explained how to use a shortcut is just 
> not good. When users click a button (unless it's a help button), they expect 
> something to happen. And still: We're putting something in the UI which is 
> used only a single time (afterwards the user knows that the function exists)
> 
> Gregor Mi wrote:
> 2. "Put it into the tooltip": Yes, sure this can and should be done. But 
> also see point 5.
> 
> 
> 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly 
> encouraged in the HIG. My mistake, thanks for clearing that!
> 
> 
> 4a. "Clicking a button": the original idea was that clicking the button 
> actually triggers the function but for current technical reasons it was 
> postponed. So, adding the menu item now might motivate someone to go a step 
> further and implement the rest. One step at a time.
> 
> 4b. "only used a single time": I myself am a user and due to the 
> outstanding stability of the desktop I keep forgetting these shortcuts. ;-) 
> Seriously, often I found myself in a situation where I knew there is a 
> shortcut but could not remember which (and previously I was lost completely 
> because I did not know that the killing window function exists at all). By 
> now I think I will never forget it, though. :)
> 
> 5. Funnily, the first time I actually saw that the shortcut is documented 
> in the toolip was when I began to change the code to prepare this RR to make 
> the shortcut more visible. :) Sure, this only a single user report but I 
> don't think I am the only who does not read the whole tooltip.
> 
> Ken Vermette wrote:
> As I see it here, the current solution isn't good and I'd argue against 
> changing things just because they feel stale. Mainly you're trading one 
> less-than-ideal solution (using a tooltip) in exhange for a solution which is 
> more complicated and bloats the UI.  Why do we need to go so far out of our 
> way to advertise XKill when we are capable of killing apps from the monitor? 
> Users of the monitor familliar with the system chould not have purely 
> informational menus burned into the main UI.
> 
> Split buttons are also abused regularly, and it's not clear what the "V" 
> menu offers until activated. Why would someone who won't stop for a tooltip 
> know to press the "V" button? What if they assume it's for other actions like 
> pause, suspend, and resuming the selected process? Users *still* won't click 
> it if that's the case. This is essentially a much more convoluted tooltip 
> which works under the assumption users will investigate it because it's 
> disguised as functionality.
> 
> I also dislike that it advertised as a feature and not a help option, and 
> users with a crashing/frozen application will only be more frusterated if 
> they think they have a solution - and are instead given a manual. Nowhere 
> else do we use this pattern. Information should NEVER be disquised as 
> functionality; it **severly** degrades user trust. Imagine if every 
> application did this - would you use a system which kept popping up with info 
> boxes instead of doing the work? Especially when you *need* the system to 
> take care of a problem and the system tells you "I can't do this, try doing 
> this" - I can think of no better way to shatter a users trust; one thing is 
> already broken, we should not make it feel 

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: Widget style and color theme problems

[Thomas Lübking]

Don't allow or set custom palettes w/ the gtk style.
GtK+ isn't colorable (and if you try to write a bright on dark style, you'll 
figure that this isn't suported in real gtk applications either)

No idea whether that's a problem in kf5 (or your application supports custom 
palettes) but if the style is gtk, one better sets the style provided palette.


Cheers,
Thomas


Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

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: robots.txt in quickgit.kde.org

[Thomas Lübking]

On Montag, 28. Dezember 2015 18:09:32 CEST, Kevin Funk wrote:

Are you aware that not even every KDE developer knows about 
LXR? I constantly have to tell people about it.



Yes, and I'm as well aware of the "if it's not in google, it doesn't exist" 
phenomenon, BUT: that's not gonna work.

If you search for QComplicatedClass, you'll find the Qt API docs, maybe some bug and most 
likely some stackoverflow entry - not a token in some thousand lines of code (leaving 
aside that mindless "i copy what I don't understand" is not the best of all 
approaches) - that will be on page [where no one has gone before]

Now consider someone outside of KDE even, trying to figure out where 
KAwesomeClass is defined


But you already know it exists?


As mentioned I don't care, but for devs, one should ensure they get aware of lxr.kde.org 
- that's *far* better than having them enter "m_foo" into google and click 
until they find a result in KDE.

The idea that someone finds a solution to his problem in the KDE sources is a 
bit far off to me.


No idea what it would take, but ideally google would simply forward lxr.kde.org when you 
search for "kde" and something else.

Stillshrug,
Thomas


Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: robots.txt in quickgit.kde.org

[Thomas Lübking]

On Sonntag, 27. Dezember 2015 12:35:51 CEST, Ben Cooksley wrote:


We could probably make it available by publishing the source trees
used by LXR / EBN.



Because if it's not in google, it doesn't exist?

We've lxr which is a dedicated and *far* superior way to search our code, so what exactly 
is the purpose of finding "m_fooBar = new KFoo::Bar()" via google? (let alone 
bing ;-P )


Cheers, sorry if I sound stupid.
Thomas


Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Policy regarding QtWebKit and QtScript

[Thomas Lübking]

On Freitag, 25. Dezember 2015 12:42:26 CEST, Milian Wolff wrote:

Sorry, but how is "it takes long to compile" and argument for or against a 
piece of software if there is no feature equivalent alternative that takes 
less time to compile?


How demanding is it exactly?
Considering Gentoo users, it could be quite a problem if it takes 32GB RAM or 
something.

Cheers,
Thomas

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
> > <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: Policy regarding QtWebKit and QtScript

[Thomas Lübking]

On Dienstag, 22. Dezember 2015 19:44:21 CEST, Aleix Pol wrote:


compiling for some time, but that's not a reason to rely on it on our
side.


Sure, getting rid of it is mandatory.
What worries me is that this *break* happens in a *minor* Qt release. Should 
generally not happen. Period.

It should still be released and everytime you try to build against it (include it) and 
didn't "#define I_KNOW_WEBKIT_IS_BITROT_AND_AM_PORTING_TO_QT_WEBENGINE_ALREADY" 
you get a compiler error.

The idea that users may have remainders of QtWebKit 5.5 on their disk (or not 
and thus unresolvable linkage) and install Qt 5.6 and still have (not 
recompiled) client code that is now gonna crash scares me a bit - it doesn't 
really improve reputation.
Distros will virtually *have* to provide downstream webkit solutions to cover 3rd party 
installs and we'll get "somthing broke" reports on this all over the place.

Sigh.
Thomas

Re: Policy regarding QtWebKit and QtScript

[Thomas Lübking]

On Dienstag, 22. Dezember 2015 19:05:23 CEST, Ivan Čukić wrote:


So, using Kross would mean implementing the kjs backend for it that we
had in 4.x times?


Isn't the designated successor for QtScript QJSEngine (I even assumed there 
should be some porting tools?)

About QtWebkit - what exactly does "disappear" mean in this context?
Will it be impossible to link QtWebKit 5.5 to Qt 5.6 ?
For if it would, that would be one giant ABI break and the answer would be to 
fork Qt... :-P

Cheers,
Thomas

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
> > 
> >
> > 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 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: Why is C90 enforced in KDE?

[Thomas Lübking]

On Sonntag, 20. Dezember 2015 04:21:29 CEST, Kevin Kofler wrote:

The kdewin team should just point people to a flex.exe that produces files 
that work with the bitrotten C compiler included with Visual C++.


So we only need somebody who forks the hardly maintained flex/yacc tools for 
windows...

It should be simpler to point windows users to a recent MSVC (2015), gcc, 
clang, or icc (ie. mandate them for KF5, raising the bar from Qt5)

We could then indeed raise to -c99 and "guarantee" compilation only for 
compatible compilers (while older MSVC *may* still work)

I'd say it's up to the kdewin people to state their preferences here
- require bison and switch to c++ in the flex/yacc toolchain
- (soft)require a c99 compliant compiler (where MSVC 2013 *may* still work)


Another, more practical, reason is that if a bug (maybe a security issue)
affecting the generated output is fixed in Flex, the pre-generated files
would not pick up the fix.


Actually, that rather supports shipping pre-generated code (given the apparent 
support situation of flex/yacc on windows - windows users could encounter flex 
bugs none of us ever sees and the backtraces make no sense...)

Cheers,
Thomas

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 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. 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: 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]

> 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/#review89351
---



src/kdeui/kdialogbuttonbox.cpp (line 36)
<https://git.reviewboard.kde.org/r/126308/#comment61103>

unrelated and not required



src/kdeui/kdialogbuttonbox.cpp (line 39)
<https://git.reviewboard.kde.org/r/126308/#comment61105>

QDialogButtonBox::addButton should do correctly anyway, so please don't 
workaround things that are not broken.



src/kdeui/kdialogbuttonbox.cpp (line 57)
<https://git.reviewboard.kde.org/r/126308/#comment61107>

you can completely spare this, there's no reason to manipulate a copy of 
the GuiItem, just burns CPU



src/kdeui/kdialogbuttonbox.cpp (line 61)
<https://git.reviewboard.kde.org/r/126308/#comment61108>

unrelated and it won't leak, since the cleanup is done by the parent/child 
relation ("this" passed to KPushButton)



src/kdeui/kdialogbuttonbox.cpp (line 70)
<https://git.reviewboard.kde.org/r/126308/#comment61106>

Setting the icon is sufficient, please do not mess around with other 
attributes.



src/kdeui/kdialogbuttonbox.cpp (line 75)
<https://git.reviewboard.kde.org/r/126308/#comment61110>

this is really the only thing you should need to do here.



src/kdeui/kpushbutton.cpp (line 257)
<https://git.reviewboard.kde.org/r/126308/#comment6>

still wrong and again, please don't mess with the icon size - you're just 
tempting DIV zero segfaults.


- Thomas Lübking


On Dec. 11, 2015, 12:59 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. 11, 2015, 12:59 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X, KDE Frameworks, Qt KDE, and Hugo 
> Pereira Da Costa.
> 
> 
> 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/kdialogbuttonbox.cpp 0f6649b 
>   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
> 
>

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]

---
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
> 
>

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 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: Change to Mail Infrastructure - SPF and DKIM verification will now be enforced

[Thomas Lübking]

> It is irrelevant what our personal preference is. The fact of life is
that there *are* mailing lists out there which perform these modifications,
and these MLs won't change their config despite changes on our side. If we
start rejecting these e-mails, well, our addresses will be unsubscribed
from these MLs

If this is about *incoming* mails, what would prevent "us" (i have no kde
address anyway) from whitelisting known mailing lists until (if ever) they
align their setups?
(technically spoken, if you modify a mail, *you* wrote it, i assume that's
why mailman has a dedicated wrapping feature)

Cheers,
Thomas

Re: Why is C90 enforced in KDE?

[Thomas Lübking]

On Montag, 7. Dezember 2015 15:54:40 CEST, Luca Beltrame wrote:

Given you've said this multiple times, with my packager hat on I'll just 
mention this: just don't make it harder *for us* to work just because 
you're targeting another platform.


I actually don't think this related at all.

Compiling C99 (beyond some minor additions like the comments, but that's not guaranteed to be the 
only usage) on MSVC is a general problem to begin with (if you care about elder versions of what MS 
calls a "compiler"), so Boudewijn's primary problem is the usage of flex/yacc to begin 
with and he'll prefer pre-translated C-fixed-to-90 (hello sed ;-) OR flex/yacc being translated to 
*.cpp (where "i build every shit and just guess what the developer meant" MSVC still 
sucks, but not that much)

Distros and notably self-builders would probably prefer such as well (less 
build time dependency, yeah!), so there's no conflict.

Otoh, developers will prefer to have flex/yacc in the CI and will require a cmake 
rule to include *.l & *.y in the source list (so it's regenerated on local 
changes) but otherwise there's (afaik) no strong reason to not simply ship the 
pre-translated sources (along the lex sources which are usually not invoked on 
build)

The situation is (afaik) slightly different w/ *.moc since you might run into "the 
moc that generated this header is too old" issues (latter happens, so we can/should 
not ship pre-built mocs; but I'm not sure whether such problems can show up with lex as 
well)

Cheers,
Thomas, Baseball cap - I've not hats.

Re: Why is C90 enforced in KDE?

[Thomas Lübking]

On Montag, 7. Dezember 2015 03:14:05 CEST, Kevin Kofler wrote:
not surprised that they are now using C99 comments (which ARE compliant to 
the current C standard, and have been for 16 years (!)).


Sure, but since it seems it's the only C99 feature used(?, stdint is more a library 
thing, at least gcc more or less tagged it as such¹), this looks like an intended break. 
At least I completely fail to see the advantage of c++ style comments for a machine 
(rather the opposite, since "dumb" appending to lines can now produce undesired 
results)

Either way, Qt5 supports down to MSVC 2010, Qt4 down to MSVC 2008, so either we
- raise the bar in kdelibs/KF5 to require a C99 compliant compiler (as a build dep 
requires it, "we just need //" isn't true - the requirements are controlled 
outside
- ship only pre-produced and hand-fixed code from flex/yacc
- pipe flex/yacc results through c++ rather than the C compiler

Cheers,
Thomas

[1] https://gcc.gnu.org/c99status.html

Re: Generated files in version control (was: Re: Why is C90 enforced in KDE?)

[Thomas Lübking]

On Montag, 7. Dezember 2015 01:08:31 CEST, Nicolás Alvarez wrote:


It will look better to stick my app logo into the real-artist-designed
piece-of-paper-with-shadow than to draw an icon from scratch...


Afaiu, one should have asked the oxygen team to avoid this kind of patched 
icons.



I don't think that's the case. Surely the
preferred way to modify an icon is to edit the SVG and rasterize it
again.


Again: that's processing. The required file was the png (since svg icons were 
initially not even supported and even now the Qt svg renderer is completely not 
up to inkscape features extending vanilla svg)


If you used gimp, you should put your multilayer .xcf


**mt** - it's not simply "multilayer", that's just an intermediate result. 
The (hypothetical) icon was forged using several destructive processes to get the pixels 
colorful in the desired way - which are not documented anywhere.
It could even haven been done in the very same layer or layer merging was one 
of the required process steps.
A rastered image really completely rests in itself - no matter what tools were 
used to forge it. A vector editor is just a process detail; the same result 
could have been achieved in MS paint (with a lot of time ;-)

Cheers,
Thomas

Re: Why is C90 enforced in KDE?

[Thomas Lübking]

On Montag, 7. Dezember 2015 17:22:47 CEST, Boudewijn Rempt wrote:


There are two sides, of course: if making it easier for a distribution
to package KDE software makes it harder for an application to be packaged
for another distribution, where do we go? What's most important? Just
adding a dependency because all linux distributions have it so it's no
sweat can cause huge problems.


Back on topic: do you actually have trouble with invoking flex/yacc* to 
generate code or do you have a problem with the generated (C99) code (what 
seems to be a pretty new problem - and is on Linux/any distro because KF5 does 
--std=c90)?

Or was this just a general "Hey, there's a world beyond Linux" remark?

You also may have some (personal, at least) data on relevant MSVC 2013 is in 
practice?
Because every gcc in the wild as well as clang should be able to handle C99 - 
and so should MSVC 2015 and the intel compiler(s of the past). Afaics, MSVC 
2013 is the deal breaker here; but no warranty on that statement...

Cheers,
Thomas

* other than "install it" - Windows doesn't come with a compiler by default 
either.

Re: Why is C90 enforced in KDE?

[Thomas Lübking]

On Sonntag, 6. Dezember 2015 16:08:04 CEST, Antonio Rojas wrote:
C90. Adding -std=c99 to the CFLAGS at compile time doesn't have any effect, 
since it is overriden by kdelibs (and by extra-cmake-modules in KF5). What 
is the reason for this?


I guess because of poor compiler support.
One needs MSVC 2013 for at least *some* C99 support :-P
(2015 should be somewhat complete, though)

Cheers,
Thomas

Re: Generated files in version control (was: Re: Why is C90 enforced in KDE?)

[Thomas Lübking]

On Sonntag, 6. Dezember 2015 22:23:01 CEST, Nicolás Alvarez wrote:


I am aware that Nuno manually chose rendering engine and scaling
method for every individual Oxygen icon based on seeing which one gave
(subjectively) better results. That is not documented anywhere


Seriously? I mean, are you saying the choices where not recorded anywhere? (To 
automize re-generation on a rule base)



I don't think there is any working implementation of RFC1437

Rumor has it that Dr. Ira Graves will have made some progress on this 
particular issue.


If I want to make an Oxygen-style icon


of vastly inferior quality ... :-P


for my application's document format, I can copy the .svg for an existing
file format icon, put my application logo, leave the paper,
and... who knows how to produce the magic .png from that. No documentation,
no script.


Is this a real world issue or is the solution being "breeze"?



I could even bring licensing into the discussion.


Afaiu you can't - png is the "source", because they were post-processed in 
addition?!?
Even if not: it doesn't matter whether you use inkscape or gimp or krita or 
illustrator or photoshop or MS paint to create a raster image (or whether you 
provide intermediate results as well) - otherwise things would get *really* 
complex, because the *entire* production process of the image would have to be 
documented to eg. allow recretation of a png drawn in gimp (Mask here, gradient 
there. Gaussian blur, 40% overlay. Flatten, displacement noise, 
yaddayaddayadda...)

IOW, iamges don't fit code licenses at all.

Cheers,
Thomas

Re: Why is C90 enforced in KDE?

[Thomas Lübking]

On Sonntag, 6. Dezember 2015 17:41:07 CEST, Nicolás Alvarez wrote:

Then we ignore warnings about // comments and pay attention
to others


Wtf does flex/yacc produce incompliant comments?
Seriously, COMMENTS!
That's a convenience thing.

int foo = bar; // bar defined in wherever

is simpler than

int foo = bar; /* bar defined in wherever */

but a code generator isn't lazy.
There must be a way to avoid that?

Otherwise one might convince them to create cpp/cxx suffixes so that compilers 
will invoke the C++ compiler?

Cheers,
Thomas

Re: MacPorts Patches

[Thomas Lübking]

On Montag, 30. November 2015 17:03:01 CEST, Martin Graesslin wrote:

There have been various OSX patches being created against the kde-workspace 
repository in the past. So yeah, it seems to be part of what they want to 
achieve.


I doubt it makes sense to run plasma-desktop/kwin/plasmashell on OSX but there 
seemed to be efforts to run X11 (+KDE) on Darwin (which is essentially just 
some BSD)

Cheers,
Thomas


Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: Causes of session management problems in Plasma 5

[Thomas Lübking]

On Dienstag, 24. November 2015 02:02:46 CEST, Andreas Hartmetz wrote:


- The session manager not "locking down" or better copying the list of
  clients *while* logging out. This would arguably only help buggy
  clients, but may still be a net positive.


It would falsely restore clients that do not want to be restored (maybe also 
because they've gotten some autostart entry)


  Why copy the list? Logout may be canceled, so it is valuable to keep
  the main client list updated for after logout cancellation.


So if I logout, kwrite exits, konsole asks, I cancel, I restart kwrite, I logout, kwrite 
exits, konsole asks, I cancel, I restart kwrite, ... I end up with n iterations of kwrite 
on the restorage? Nahhh the SM should restore what the user left behind on logout, 
not what he had running in the former session "somewhen" ;-)

Other than that, thanks and kudos for the Investigation and get QGuiApplication 
fixed itr. =)

Cheers,
Thomas

Re: Causes of session management problems in Plasma 5

[Thomas Lübking]

On Donnerstag, 26. November 2015 05:19:34 CEST, Nicolás Alvarez wrote:

What do you mean with "konsole asks"? Things like "You have 
multiple tabs open, are you sure you want to quit?" and "You 
have unsaved changes"?


Yes.

If so, the scenario you describe is bad regardless of session 
restoration.


Yes. Unfortunaltely.

If konsole has to ask the user and the user has a 
chance to say no and cancel the logout at that point, then 
kwrite shouldn't have exited yet!


That's what the spec says, but the ksmserver change suggestion is about buggy 
clients that behave exactly this way.

If "app canceling logout" is a thing, then logout should feel 
transactional to the user. Either logout happens and all apps 
exit, or logout doesn't happen and nothing exits.


Yes. That's what the spec says ...


I guess the implementation of that would be: all apps should be given a 
chance to ask their questions and approve or stop the logout


That'S exactly what happens, but ...


before *any* app exits.


We cannot stop process from doing stupid things, like exiting in the wrong 
moment.


This is how MS Windows works, by the way (or used to work).

No, it's how not buggy applications work on windows.

Cheers,
Thomas

Re: Causes of session management problems in Plasma 5

[Thomas Lübking]

On Donnerstag, 26. November 2015 00:37:35 CEST, Andreas Hartmetz wrote:

First, by copy I mean a temporary copy that is never merged back into 
the main list, it's kept around only to know which processes have 
already agreed to have their session saved and submitted the 
corresponding data.


The current problem is that some applications die between submitting 
their state to ksmserver (SaveYourselfDone message) and ksmserver saving 
the list of processes to restart (writing ksmserverrc).


Ok, that's not a problem.

It is AFAICS 
safe to assume that an application that has submitted its session data 
is really supposed to be restored.


Agreed, and if this is only meant as temporary condition we won't restart a 
session history either. Given the async nature of IPC the cover-up sounds 
reasonable enough, yes.

Cheers,
Thomas

Re: Kubuntu 15.04 Graphics Driver Issue

[Thomas Lübking]

On Montag, 23. November 2015 07:50:43 CEST, gokul krishna wrote:

ensure to blacklist the nouveau kernel module, drop the framebuffer from GRUB 
[1] and notice that only the most recent nvidia drivers are ABI compatible w/ 
Xorg 1.18

Also remove "nomodeset" and "splash" from your config (the splash might 
actually be the troublemaker, turning off kms globally is neither required nor a good idea)


This has nothing to do with KDE (you deleted your configs for no reason) and this is a 
developer mailing list, end-user help (even completely off topic) is usually provided on 
forum.kde.org. Please head over there for further assistance and ensure to provide 
/var/log/Xorg.0.log and the output of "dmesg"

Cheers,
Thomas

[1] 
https://wiki.archlinux.org/index.php/GRUB/Tips_and_tricks#Disable_framebuffer


Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

Re: [Kde-hardware-devel] Review Request 126146: [XRandRBrightness] Don't call for xrandr if it's not available

[Thomas Lübking]

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


+1
Functional patch looks fine, since the extension is not gonna mystically show 
up, one may wish to cache extension status validity in a tenary member (iff 
this constructor is called more often)

Can't tell about "unrelated changes policy" in the component ;-)

- Thomas Lübking


On Nov. 23, 2015, 4 nachm., Kai Uwe Broulik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126146/
> ---
> 
> (Updated Nov. 23, 2015, 4 nachm.)
> 
> 
> Review request for Solid and Thomas Lübking.
> 
> 
> Bugs: 352462
> https://bugs.kde.org/show_bug.cgi?id=352462
> 
> 
> Repository: powerdevil
> 
> 
> Description
> ---
> 
> Check whether the extension is there before calling into it.
> 
> Also cache calls to QX11Info::connection() if we're at it.
> 
> 
> Diffs
> -
> 
>   daemon/backends/upower/xrandrbrightness.cpp 0abcefe 
> 
> Diff: https://git.reviewboard.kde.org/r/126146/diff/
> 
> 
> Testing
> ---
> 
> Still works, didn't test without xrandr.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

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

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