KCModule: Indicate when a setting has been changed from the default or previous value
ClosedPublic

Authored by ervin on Feb 21 2020, 11:03 AM.

Details

Summary

Extend KConfigDialogManager internals to insert a small
SettingsStatusIndicator widget next to widgets representing a setting
which is differs from default value.

Test Plan

Tested it with several widgets based KCMs, namely: qtquicksettings,
desktoppath and screenlocker. Those three have different situations
in term of layouts. Worst case scenario is desktoppath which leads
to the right hand side of the fields moving a bit to fit the indicators
but that's the price to pay for the feature I guess while keeping
things readable.

Here is a screenshot how of it looks on the qtquicksettings KCM:

Note that the indicators are clickable and reset the field to default value.

Diff Detail

Repository
R265 KConfigWidgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven added inline comments.Feb 21 2020, 11:24 AM
src/kconfigdialogmanager_p.h
61

I would have named those with s : indicatorWidgets, knownWidgets, buddyWidgets

src/settingsstatusindicator_p.h
42

Make setChanged and setDefaulted as slots to ease the reuse of this class outside of KConfigDialogManager

How does it look? Will this be opt in for other users of KConfigDialogManager?

ervin added inline comments.Feb 21 2020, 12:07 PM
src/kconfigdialogmanager_p.h
61

Not gonna rename those though, I'm merely moving code around (apart from indicatorWidget I could at least rename that one indeed).

The editmarks should probably respect the visibility of the associated widget. In this picture I use a invisble lineedit to manage the settings of the QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

The editmarks should probably respect the visibility of the associated widget. In this picture I use a invisble lineedit to manage the settings of the QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)

Very good point!

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

I use KConfigDialogManager to manage the settings in Spectacle MainWindow which are instant apply by
connect(mConfigManager, &KConfigDialogManager::widgetModified, mConfigManager, &KConfigDialogManager::updateSettings); and I don't like it that they appear there too

a setting which is currently dirty or which differs from default value.

What is your idea behind this? After first seeing this patch my intuition was that it would show for unsaved changes. But then I was confused when I open a SettingsDialog that there were already marks even though I changed nothing! I would expect them only for unsaved changes, I don't know how other platforms handle this if they have something similiar?

ervin added a comment.Feb 21 2020, 1:11 PM

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

I use KConfigDialogManager to manage the settings in Spectacle MainWindow which are instant apply by
connect(mConfigManager, &KConfigDialogManager::widgetModified, mConfigManager, &KConfigDialogManager::updateSettings); and I don't like it that they appear there too

You mean they appear and stick around? Or they appear/disappear immediately?
If they stick around I'd indeed have to check why the state isn't recomputed on the updateSettings call...

a setting which is currently dirty or which differs from default value.

What is your idea behind this? After first seeing this patch my intuition was that it would show for unsaved changes. But then I was confused when I open a SettingsDialog that there were already marks even though I changed nothing! I would expect them only for unsaved changes, I don't know how other platforms handle this if they have something similiar?

Currently it helps the user find out the unsaved changes (the least useful in some way, or your immediate memory is not good) but it also helps the user find out which of her settings differ from the default values (and that's indeed something you will forget over time and wonder "why the hell is the defaults button enabled").

Hope the extra context helps.

How does it look? Will this be opt in for other users of KConfigDialogManager?

I'd say you should try it. ;-)
I really need wider feedback on how it behaves in different context. Currently the patch makes it mandatory for everyone.

I use KConfigDialogManager to manage the settings in Spectacle MainWindow which are instant apply by
connect(mConfigManager, &KConfigDialogManager::widgetModified, mConfigManager, &KConfigDialogManager::updateSettings); and I don't like it that they appear there too

You mean they appear and stick around? Or they appear/disappear immediately?
If they stick around I'd indeed have to check why the state isn't recomputed on the updateSettings call...

a setting which is currently dirty or which differs from default value.

What is your idea behind this? After first seeing this patch my intuition was that it would show for unsaved changes. But then I was confused when I open a SettingsDialog that there were already marks even though I changed nothing! I would expect them only for unsaved changes, I don't know how other platforms handle this if they have something similiar?

Currently it helps the user find out the unsaved changes (the least useful in some way, or your immediate memory is not good) but it also helps the user find out which of her settings differ from the default values (and that's indeed something you will forget over time and wonder "why the hell is the defaults button enabled").

Hope the extra context helps.

They stick around because as you said the settings differ from the defaults.

The pen for me conveys the meaning that something was edited that's the reason I would only show it for unsaved changes. But let's others weigh in what they think

Please add screenshots to the Test Plan section for patches that change the UI. :) Also it would be nice to indicate how someone can test this. Which apps? System Settings? Where? How?

ervin added a comment.Feb 21 2020, 2:22 PM

Please add screenshots to the Test Plan section for patches that change the UI. :) Also it would be nice to indicate how someone can test this. Which apps? System Settings? Where? How?

Well, second part I pointed out a few kcms you can test with. ;-)

ervin added a comment.Feb 21 2020, 2:24 PM

They stick around because as you said the settings differ from the defaults.

Right, so "not a bug" (as in that's what's the patch is intended to do so far).

The pen for me conveys the meaning that something was edited that's the reason I would only show it for unsaved changes. But let's others weigh in what they think

Right, I picked visual indicators which came to mind, I'm totally open to suggestions on which would be better suited.

ervin added a comment.Feb 25 2020, 1:43 PM

The editmarks should probably respect the visibility of the associated widget. In this picture I use a invisble lineedit to manage the settings of the QButtonGroup beneath (QButtonGroup isn't supported by KCOnfigDialogManager)

For the record, next iteration of that patch will honor the visibility of the associated widget (got some more changes to do locally before uploading though, stay tuned).

That being said, what you did is a very naughty hack you know. Why not fixing KConfigDialogManager or go through a QGroupBox (which is handled in some way). ;-)

ervin updated this revision to Diff 76362.Feb 25 2020, 1:57 PM

Handle visibility of the tracked widget, setters become slots, and add plural to "indicatorWidgets".

ervin edited the test plan for this revision. (Show Details)Feb 25 2020, 2:00 PM

Please add screenshots to the Test Plan section for patches that change the UI. :) Also it would be nice to indicate how someone can test this. Which apps? System Settings? Where? How?

Well, second part I pointed out a few kcms you can test with. ;-)

And now you got a screenshot as well. Waiting for further feedback now.

ngraham retitled this revision from Status indicator for individual widgets in KCModule to KCModule: Indicate when a setting has been changed from the default value.Feb 25 2020, 2:24 PM

That title is now not optimal isn't it? 1. It's not only in KCModule 2. It also shows unsaved settings

ngraham retitled this revision from KCModule: Indicate when a setting has been changed from the default value to KCModule: Indicate when a setting has been changed from the default or previous value.Feb 25 2020, 2:40 PM
ndavis added a subscriber: ndavis.EditedFeb 25 2020, 2:43 PM

I can see the utility of indicating non-default values in some cases, particularly with software that has a lot of necessary complexity in the settings or where non-default values can cause problems (see SVG Cleaner GUI for an example of both). However, I don't think it's necessary for most software to indicate non-default values or even dirty values.

Here's a picture of SVG Cleaner GUI for reference:

Notice how the non-default values are indicated with a blue dot and categories that contain options with non-default values also have a blue dot.

alexde added a subscriber: alexde.Feb 25 2020, 3:06 PM

Does this patch also fix indirectly #274629?

ervin added a comment.Feb 25 2020, 3:16 PM

Does this patch also fix indirectly #274629?

I doubt it, this bug report seems confused BTW... it was never broken on the kdelibs side but *some* dialogs have bugs which break the feature for them. Unrelated to this patch we'd been doing a big sweep on the systemsettings module to address among other thing the problems they might have in that department.

ervin added a comment.Mar 13 2020, 8:51 AM

And now you got a screenshot as well. Waiting for further feedback now.

Ping? This patch was first created three weeks ago now.

Note that look in D27840 and D27841 is pretty much the same than in here for which I provided screenshot

@ndavis I know you had some idea for which icon to use, and an idea to make it a clickable button, right?

ndavis added a comment.EditedMar 15 2020, 2:21 AM

@ndavis I know you had some idea for which icon to use, and an idea to make it a clickable button, right?

Right, it's like MuseScore:

  • It's simple to understand with the right iconography.
  • It's convenient when you're working with a lot of settings that you don't necessarily fully understand.
  • By being enabled or disabled, it indicates whether or not a control is set to the default value or not.

Some extra rules I thought of:

  • With the checkable label example in the mockup above, it should reset both the label and the checkbox.
  • In cases where there is no default value and a user is expected/required to change the value (e.g., LineEdits for the user's first and last names), we should not show a reset button.
    • I don't want users to mistake it for a clear text or undo button.
  • When the reset button is disabled, it should be invisible to reduce visual clutter.
ervin added a comment.Mar 17 2020, 3:44 PM

Some extra rules I thought of:

  • With the checkable label example in the mockup above, it should reset both the label and the checkbox.

Just for the record, this will unlikely to be enforceable on the framework side of the code, likely to be taken care of on a case by case basis when we encounter those in the modules themselves. Just managing expectations on what the code can easily do at that level of abstraction. So don't look for it in the patches I submit around the topic. ;-)

The rest should be doable I'll update my patches shortly.

ervin updated this revision to Diff 77847.Mar 17 2020, 5:00 PM

Take feedback about the GUI into account

ervin edited the summary of this revision. (Show Details)Mar 17 2020, 5:01 PM
ervin edited the test plan for this revision. (Show Details)
ervin edited the test plan for this revision. (Show Details)

Is it possible to align all of the reset buttons like a column?

ervin added a comment.Mar 17 2020, 5:39 PM

Is it possible to align all of the reset buttons like a column?

Why I'm not surprised. ;-)

Honestly with enough code it might be possible, but that'd be expensive in term of effort... I'll need to keep track of all widgets in the page. I'll give it a shot but don't hold your breath.

ervin updated this revision to Diff 78468.Mar 25 2020, 3:51 PM

Have the indicators vertically line up automatically

ervin edited the test plan for this revision. (Show Details)Mar 25 2020, 3:52 PM
ervin added a comment.Mar 25 2020, 3:55 PM

Is it possible to align all of the reset buttons like a column?

Why I'm not surprised. ;-)

Honestly with enough code it might be possible, but that'd be expensive in term of effort... I'll need to keep track of all widgets in the page. I'll give it a shot but don't hold your breath.

Alright, turns out I managed to do it both for QtWidgets and QtQuick cases, so you can breath again. ;-)

Reviews on all my patches very welcome.

ervin updated this revision to Diff 78703.Mar 27 2020, 8:57 PM

As advised by Kai and David on D27840, switch to using tool buttons and fix RTL handling.

What do we want to happen for released code that gets a bugfix update?

src/kconfigdialogmanager.cpp
609

Why not item->readDefault()?

625

won't it do it itself when the property changes?

src/settingsstatusindicator.cpp
76

This is equivalent to calling showFullScreen(), showMaximized(), or setVisible(true), depending on the platform's default behavior for the window flags.

For X and wayland it's setVisible(true)

but we shouldn't count on it.

176

unused?

185

Can we be sure the tracked widget always has a parent widget?

If someone doesn't use layouts a widget might not have a parent.

193

that's not true for the RTL case where the widget is expected to resize.

It would be w->pos().x() + w->width() - widgetExpectedWidth(w)

ndavis added a comment.EditedMar 31 2020, 3:36 PM

Somehow I missed the notification that this was updated.

Thanks for the horizontal alignment. Could you also add a left margin to the column of reset buttons? It should be the same as the spacing between the labels and the controls, which is equivalent to Kirigami.Units.smallSpacing, IIRC.

ervin edited the test plan for this revision. (Show Details)Apr 8 2020, 4:33 PM

Somehow I missed the notification that this was updated.

Thanks for the horizontal alignment. Could you also add a left margin to the column of reset buttons? It should be the same as the spacing between the labels and the controls, which is equivalent to Kirigami.Units.smallSpacing, IIRC.

Actually was already the case, forgot to update the screenshot.

ervin marked 6 inline comments as done.Apr 8 2020, 4:55 PM
ervin added inline comments.
src/kconfigdialogmanager.cpp
609

Wouldn't do the same thing at all. readDefault() takes a KConfig object and updates the default value stored in the item with what it found in there.

Yes, I know... the item API is weird...

625

Good point, was indeed unnecessary now, I removed the line.

src/settingsstatusindicator.cpp
76

I don't think it matters for widgets which have a parent and not the Qt::Window window flag, but OK, switching to setVisible() instead of show/hide.

176

I'm not sure how you end up to this conclusion, it's written two below if we are at the window edge, and it's used in the move call at the end.

185

Well that'd mean the tracked widget is a window... it's pretty much an impossibility IMO.

193

Either I misunderstood what you meant or you got the math wrong on that one.

What you're proposing (or what I understood you're proposing) breaks the "RTL + widget at edge" case in my tests. The line I wrote is working perfectly fine for my tests with desktoppath and qtquicksettings both in LTR and RTL modes.

ervin updated this revision to Diff 79656.Apr 8 2020, 4:56 PM
ervin marked 6 inline comments as done.

Addresses David's comments

davidedmundson accepted this revision.Apr 20 2020, 2:32 PM

Assuming VDG are happy, go for it.

This revision is now accepted and ready to land.Apr 20 2020, 2:32 PM

David asked for VDG to approve before this landed, which wasn't done.

I have concerns about this. The button has no tooltip so it's not obvious what will happen when clicked on. It should at least say "Revert to default setting" in a tooltip, and preferably even the name or text of the default setting that will be reverted to.

Also, we now have a new inconsistency in that a small fraction of System Settings KCMs and app settings windows will display these indicators, but not others.

Finally clicking on the revert button in Spectacle's settings page causes a segfault for me:

1#0 KConfigDialogManagerPrivate::updateWidgetIndicator (this=this@entry=0x13324b0,
2 configId=..., widget=widget@entry=0x1234e50)
3 at /home/nate/kde/src/kconfigwidgets/src/kconfigdialogmanager.cpp:613
4#1 0x00007ffff76d2f5f in KConfigDialogManagerPrivate::onWidgetModified (this=0x13324b0)
5 at /home/nate/kde/src/kconfigwidgets/src/kconfigdialogmanager.cpp:598
6#2 0x00007ffff5eeb9fe in QtPrivate::QSlotObjectBase::call (a=0x7fffffffadc0,
7 r=0x132ca20, this=0x1334b50)
8 at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
9#3 doActivate<false> (sender=0x1234e50, signal_index=11, argv=0x7fffffffadc0)
10 at kernel/qobject.cpp:3870
11#4 0x00007ffff5ee61bf in QMetaObject::activate (sender=sender@entry=0x1234e50,
12 m=m@entry=0x7ffff725c6e0 <QAbstractButton::staticMetaObject>,
13 local_signal_index=local_signal_index@entry=4, argv=argv@entry=0x7fffffffadc0)
14 at kernel/qobject.cpp:3930
15#5 0x00007ffff6e4ca92 in QAbstractButton::toggled (this=this@entry=0x1234e50,
16 _t1=<optimized out>, _t1@entry=false) at .moc/moc_qabstractbutton.cpp:320
17#6 0x00007ffff6e4ce92 in QAbstractButtonPrivate::emitToggled (this=this@entry=0x690b00,
18 checked=checked@entry=false) at widgets/qabstractbutton.cpp:457
19#7 0x00007ffff6e4e479 in QAbstractButton::setChecked (this=0x1234e50,
20 checked=<optimized out>) at widgets/qabstractbutton.cpp:650
21#8 0x00007ffff6e4e578 in QAbstractButton::setChecked (this=0x1234cf0,
22 checked=checked@entry=true) at widgets/qabstractbutton.cpp:648
23#9 0x00007ffff76d2077 in KConfigDialogManager::setProperty (this=<optimized out>,
24 w=<optimized out>, v=...) at /usr/include/qt5/QtCore/qlist.h:117
25#10 0x00007ffff76d23e1 in KConfigDialogManagerPrivate::<lambda()>::operator() (
26 __closure=0x13345e0)
27 at /home/nate/kde/src/kconfigwidgets/src/kconfigdialogmanager.cpp:624
28#11 QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, KConfigDialogManagerPrivate::updateWidgetIndicator(const QString&, QWidget*)::<lambda()> >::call (
29 arg=<optimized out>, f=...) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:146
30#12 QtPrivate::Functor<KConfigDialogManagerPrivate::updateWidgetIndicator(const QString&, QWidget*)::<lambda()>, 0>::call<QtPrivate::List<>, void> (arg=<optimized out>, f=...)
31 at /usr/include/qt5/QtCore/qobjectdefs_impl.h:256
32#13 QtPrivate::QFunctorSlotObject<KConfigDialogManagerPrivate::updateWidgetIndicator(const QString&, QWidget*)::<lambda()>, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (which=<optimized out>, this_=0x13345d0,
33 r=<optimized out>, a=<optimized out>, ret=<optimized out>)
34 at /usr/include/qt5/QtCore/qobjectdefs_impl.h:439
35#14 0x00007ffff5eeb9fe in QtPrivate::QSlotObjectBase::call (a=0x7fffffffb080,
36 r=0x1234860, this=0x13345d0)
37 at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
38#15 doActivate<false> (sender=0x1332ef0, signal_index=9, argv=0x7fffffffb080)
39 at kernel/qobject.cpp:3870
40#16 0x00007ffff5ee61bf in QMetaObject::activate (sender=sender@entry=0x1332ef0,
41 m=m@entry=0x7ffff725c6e0 <QAbstractButton::staticMetaObject>,
42 local_signal_index=local_signal_index@entry=2, argv=argv@entry=0x7fffffffb080)
43 at kernel/qobject.cpp:3930
44#17 0x00007ffff6e4ca32 in QAbstractButton::clicked (this=this@entry=0x1332ef0,
45--Type <RET> for more, q to quit, c to continue without paging--
46 _t1=<optimized out>) at .moc/moc_qabstractbutton.cpp:313
47#18 0x00007ffff6e4cc4a in QAbstractButtonPrivate::emitClicked (this=this@entry=0x1332f40)
48 at widgets/qabstractbutton.cpp:415
49#19 0x00007ffff6e4dfef in QAbstractButtonPrivate::click (this=0x1332f40)
50 at widgets/qabstractbutton.cpp:408
51#20 0x00007ffff6e4e1b5 in QAbstractButton::mouseReleaseEvent (this=0x1332ef0, e=
52 0x7fffffffb630) at widgets/qabstractbutton.cpp:1012
53#21 0x00007ffff6f3f7da in QToolButton::mouseReleaseEvent (this=<optimized out>,
54 e=<optimized out>) at widgets/qtoolbutton.cpp:622
55#22 0x00007ffff6d995be in QWidget::event (this=0x1332ef0, event=0x7fffffffb630)
56 at kernel/qwidget.cpp:8675
57#23 0x00007ffff6e4f3d3 in QAbstractButton::event (this=this@entry=0x1332ef0,
58 e=e@entry=0x7fffffffb630) at widgets/qabstractbutton.cpp:969
59#24 0x00007ffff6f3f884 in QToolButton::event (this=0x1332ef0, event=0x7fffffffb630)
60 at widgets/qtoolbutton.cpp:1002
61#25 0x00007ffff6d56caf in QApplicationPrivate::notify_helper (this=this@entry=0x4c2550,
62 receiver=receiver@entry=0x1332ef0, e=e@entry=0x7fffffffb630)
63 at kernel/qapplication.cpp:3684
64#26 0x00007ffff6d60043 in QApplication::notify (this=<optimized out>,
65 receiver=0x1332ef0, e=0x7fffffffb630) at kernel/qapplication.cpp:3128
66#27 0x00007ffff5eb7002 in QCoreApplication::notifyInternal2 (receiver=0x1332ef0,
67 event=0x7fffffffb630) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:153
68#28 0x00007ffff6d5f123 in QApplicationPrivate::sendMouseEvent (
69 receiver=receiver@entry=0x1332ef0, event=event@entry=0x7fffffffb630,
70 alienWidget=alienWidget@entry=0x1332ef0, nativeWidget=0x11cce00,
71 buttonDown=buttonDown@entry=0x7ffff72869a0 <qt_button_down>, lastMouseReceiver=...,
72 spontaneous=true, onlyDispatchEnterLeave=false) at kernel/qapplication.cpp:2614
73#29 0x00007ffff6db45f9 in QWidgetWindow::handleMouseEvent (this=0x13ac1c0,
74 event=0x7fffffffbab0) at /usr/include/c++/9/bits/atomic_base.h:413
75#30 0x00007ffff6db7694 in QWidgetWindow::event (event=0x7fffffffbab0, this=0x13ac1c0)
76 at kernel/qwidgetwindow.cpp:295
77#31 QWidgetWindow::event (this=0x13ac1c0, event=0x7fffffffbab0)
78 at kernel/qwidgetwindow.cpp:238
79#32 0x00007ffff6d56caf in QApplicationPrivate::notify_helper (this=this@entry=0x4c2550,
80 receiver=receiver@entry=0x13ac1c0, e=e@entry=0x7fffffffbab0)
81 at kernel/qapplication.cpp:3684
82#33 0x00007ffff6d5fdf0 in QApplication::notify (this=0x7fffffffc2d0, receiver=0x13ac1c0,
83 e=0x7fffffffbab0) at kernel/qapplication.cpp:3430
84#34 0x00007ffff5eb7002 in QCoreApplication::notifyInternal2 (receiver=0x13ac1c0,
85 event=0x7fffffffbab0) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:153
86#35 0x00007ffff64944d3 in QGuiApplicationPrivate::processMouseEvent (e=e@entry=0x13f9970)
87 at kernel/qguiapplication.cpp:2209
88#36 0x00007ffff6495b65 in QGuiApplicationPrivate::processWindowSystemEvent (
89 e=e@entry=0x13f9970) at kernel/qguiapplication.cpp:1941
90#37 0x00007ffff646f53b in QWindowSystemInterface::sendWindowSystemEvents (
91 flags=flags@entry=...) at kernel/qwindowsysteminterface.cpp:1163
92#38 0x00007ffff1cfca6a in xcbSourceDispatch (source=source@entry=0x587b60)
93 at qxcbeventdispatcher.cpp:105
94--Type <RET> for more, q to quit, c to continue without paging--
95#39 0x00007ffff417b048 in g_main_dispatch (context=0x7fffec005000)
96 at ../glib/gmain.c:3216
97#40 g_main_context_dispatch (context=context@entry=0x7fffec005000)
98 at ../glib/gmain.c:3881
99#41 0x00007ffff417b3d0 in g_main_context_iterate (context=context@entry=0x7fffec005000,
100 block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
101 at ../glib/gmain.c:3954
102#42 0x00007ffff417b45f in g_main_context_iteration (context=0x7fffec005000,
103 may_block=may_block@entry=1) at ../glib/gmain.c:4015
104#43 0x00007ffff5f0dbee in QEventDispatcherGlib::processEvents (this=0x58ec30, flags=...)
105 at kernel/qeventdispatcher_glib.cpp:423
106#44 0x00007ffff5eb5b9b in QEventLoop::exec (this=this@entry=0x7fffffffbe50, flags=...,
107 flags@entry=...) at ../../include/QtCore/../../src/corelib/global/qflags.h:136
108#45 0x00007ffff5ebd972 in QCoreApplication::exec ()
109 at ../../include/QtCore/../../src/corelib/global/qflags.h:118
110#46 0x00007ffff648856c in QGuiApplication::exec () at kernel/qguiapplication.cpp:1866
111#47 0x00007ffff6d56c25 in QApplication::exec () at kernel/qapplication.cpp:2824
112#48 0x0000000000423cce in main (argc=<optimized out>, argv=<optimized out>)
113 at /home/nate/kde/src/spectacle/src/Main.cpp:166

ervin added a comment.Apr 20 2020, 3:22 PM

David asked for VDG to approve before this landed, which wasn't done.

Dude, I jumped through all the hoops for the past weeks. Also it got no further reply after I updated the screenshot almost two weeks ago so yes I assumed you guys had nothing else.

ervin added a comment.Apr 20 2020, 4:26 PM

Finally clicking on the revert button in Spectacle's settings page causes a segfault for me:

1#0 KConfigDialogManagerPrivate::updateWidgetIndicator (this=this@entry=0x13324b0,
2 configId=..., widget=widget@entry=0x1234e50)
3 at /home/nate/kde/src/kconfigwidgets/src/kconfigdialogmanager.cpp:613
4#1 0x00007ffff76d2f5f in KConfigDialogManagerPrivate::onWidgetModified (this=0x13324b0)
5 at /home/nate/kde/src/kconfigwidgets/src/kconfigdialogmanager.cpp:598
6#2 0x00007ffff5eeb9fe in QtPrivate::QSlotObjectBase::call (a=0x7fffffffadc0,
7 r=0x132ca20, this=0x1334b50)
8 at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
9#3 doActivate<false> (sender=0x1234e50, signal_index=11, argv=0x7fffffffadc0)
10 at kernel/qobject.cpp:3870
11#4 0x00007ffff5ee61bf in QMetaObject::activate (sender=sender@entry=0x1234e50,
12 m=m@entry=0x7ffff725c6e0 <QAbstractButton::staticMetaObject>,
13 local_signal_index=local_signal_index@entry=4, argv=argv@entry=0x7fffffffadc0)
14 at kernel/qobject.cpp:3930
15#5 0x00007ffff6e4ca92 in QAbstractButton::toggled (this=this@entry=0x1234e50,
16 _t1=<optimized out>, _t1@entry=false) at .moc/moc_qabstractbutton.cpp:320
17#6 0x00007ffff6e4ce92 in QAbstractButtonPrivate::emitToggled (this=this@entry=0x690b00,
18 checked=checked@entry=false) at widgets/qabstractbutton.cpp:457
19#7 0x00007ffff6e4e479 in QAbstractButton::setChecked (this=0x1234e50,
20 checked=<optimized out>) at widgets/qabstractbutton.cpp:650
21#8 0x00007ffff6e4e578 in QAbstractButton::setChecked (this=0x1234cf0,
22 checked=checked@entry=true) at widgets/qabstractbutton.cpp:648
23#9 0x00007ffff76d2077 in KConfigDialogManager::setProperty (this=<optimized out>,
24 w=<optimized out>, v=...) at /usr/include/qt5/QtCore/qlist.h:117
25#10 0x00007ffff76d23e1 in KConfigDialogManagerPrivate::<lambda()>::operator() (
26 __closure=0x13345e0)
27 at /home/nate/kde/src/kconfigwidgets/src/kconfigdialogmanager.cpp:624
28#11 QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, KConfigDialogManagerPrivate::updateWidgetIndicator(const QString&, QWidget*)::<lambda()> >::call (
29 arg=<optimized out>, f=...) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:146
30#12 QtPrivate::Functor<KConfigDialogManagerPrivate::updateWidgetIndicator(const QString&, QWidget*)::<lambda()>, 0>::call<QtPrivate::List<>, void> (arg=<optimized out>, f=...)
31 at /usr/include/qt5/QtCore/qobjectdefs_impl.h:256
32#13 QtPrivate::QFunctorSlotObject<KConfigDialogManagerPrivate::updateWidgetIndicator(const QString&, QWidget*)::<lambda()>, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (which=<optimized out>, this_=0x13345d0,
33 r=<optimized out>, a=<optimized out>, ret=<optimized out>)
34 at /usr/include/qt5/QtCore/qobjectdefs_impl.h:439
35#14 0x00007ffff5eeb9fe in QtPrivate::QSlotObjectBase::call (a=0x7fffffffb080,
36 r=0x1234860, this=0x13345d0)
37 at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
38#15 doActivate<false> (sender=0x1332ef0, signal_index=9, argv=0x7fffffffb080)
39 at kernel/qobject.cpp:3870
40#16 0x00007ffff5ee61bf in QMetaObject::activate (sender=sender@entry=0x1332ef0,
41 m=m@entry=0x7ffff725c6e0 <QAbstractButton::staticMetaObject>,
42 local_signal_index=local_signal_index@entry=2, argv=argv@entry=0x7fffffffb080)
43 at kernel/qobject.cpp:3930
44#17 0x00007ffff6e4ca32 in QAbstractButton::clicked (this=this@entry=0x1332ef0,
45--Type <RET> for more, q to quit, c to continue without paging--
46 _t1=<optimized out>) at .moc/moc_qabstractbutton.cpp:313
47#18 0x00007ffff6e4cc4a in QAbstractButtonPrivate::emitClicked (this=this@entry=0x1332f40)
48 at widgets/qabstractbutton.cpp:415
49#19 0x00007ffff6e4dfef in QAbstractButtonPrivate::click (this=0x1332f40)
50 at widgets/qabstractbutton.cpp:408
51#20 0x00007ffff6e4e1b5 in QAbstractButton::mouseReleaseEvent (this=0x1332ef0, e=
52 0x7fffffffb630) at widgets/qabstractbutton.cpp:1012
53#21 0x00007ffff6f3f7da in QToolButton::mouseReleaseEvent (this=<optimized out>,
54 e=<optimized out>) at widgets/qtoolbutton.cpp:622
55#22 0x00007ffff6d995be in QWidget::event (this=0x1332ef0, event=0x7fffffffb630)
56 at kernel/qwidget.cpp:8675
57#23 0x00007ffff6e4f3d3 in QAbstractButton::event (this=this@entry=0x1332ef0,
58 e=e@entry=0x7fffffffb630) at widgets/qabstractbutton.cpp:969
59#24 0x00007ffff6f3f884 in QToolButton::event (this=0x1332ef0, event=0x7fffffffb630)
60 at widgets/qtoolbutton.cpp:1002
61#25 0x00007ffff6d56caf in QApplicationPrivate::notify_helper (this=this@entry=0x4c2550,
62 receiver=receiver@entry=0x1332ef0, e=e@entry=0x7fffffffb630)
63 at kernel/qapplication.cpp:3684
64#26 0x00007ffff6d60043 in QApplication::notify (this=<optimized out>,
65 receiver=0x1332ef0, e=0x7fffffffb630) at kernel/qapplication.cpp:3128
66#27 0x00007ffff5eb7002 in QCoreApplication::notifyInternal2 (receiver=0x1332ef0,
67 event=0x7fffffffb630) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:153
68#28 0x00007ffff6d5f123 in QApplicationPrivate::sendMouseEvent (
69 receiver=receiver@entry=0x1332ef0, event=event@entry=0x7fffffffb630,
70 alienWidget=alienWidget@entry=0x1332ef0, nativeWidget=0x11cce00,
71 buttonDown=buttonDown@entry=0x7ffff72869a0 <qt_button_down>, lastMouseReceiver=...,
72 spontaneous=true, onlyDispatchEnterLeave=false) at kernel/qapplication.cpp:2614
73#29 0x00007ffff6db45f9 in QWidgetWindow::handleMouseEvent (this=0x13ac1c0,
74 event=0x7fffffffbab0) at /usr/include/c++/9/bits/atomic_base.h:413
75#30 0x00007ffff6db7694 in QWidgetWindow::event (event=0x7fffffffbab0, this=0x13ac1c0)
76 at kernel/qwidgetwindow.cpp:295
77#31 QWidgetWindow::event (this=0x13ac1c0, event=0x7fffffffbab0)
78 at kernel/qwidgetwindow.cpp:238
79#32 0x00007ffff6d56caf in QApplicationPrivate::notify_helper (this=this@entry=0x4c2550,
80 receiver=receiver@entry=0x13ac1c0, e=e@entry=0x7fffffffbab0)
81 at kernel/qapplication.cpp:3684
82#33 0x00007ffff6d5fdf0 in QApplication::notify (this=0x7fffffffc2d0, receiver=0x13ac1c0,
83 e=0x7fffffffbab0) at kernel/qapplication.cpp:3430
84#34 0x00007ffff5eb7002 in QCoreApplication::notifyInternal2 (receiver=0x13ac1c0,
85 event=0x7fffffffbab0) at ../../include/QtCore/../../src/corelib/kernel/qobject.h:153
86#35 0x00007ffff64944d3 in QGuiApplicationPrivate::processMouseEvent (e=e@entry=0x13f9970)
87 at kernel/qguiapplication.cpp:2209
88#36 0x00007ffff6495b65 in QGuiApplicationPrivate::processWindowSystemEvent (
89 e=e@entry=0x13f9970) at kernel/qguiapplication.cpp:1941
90#37 0x00007ffff646f53b in QWindowSystemInterface::sendWindowSystemEvents (
91 flags=flags@entry=...) at kernel/qwindowsysteminterface.cpp:1163
92#38 0x00007ffff1cfca6a in xcbSourceDispatch (source=source@entry=0x587b60)
93 at qxcbeventdispatcher.cpp:105
94--Type <RET> for more, q to quit, c to continue without paging--
95#39 0x00007ffff417b048 in g_main_dispatch (context=0x7fffec005000)
96 at ../glib/gmain.c:3216
97#40 g_main_context_dispatch (context=context@entry=0x7fffec005000)
98 at ../glib/gmain.c:3881
99#41 0x00007ffff417b3d0 in g_main_context_iterate (context=context@entry=0x7fffec005000,
100 block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
101 at ../glib/gmain.c:3954
102#42 0x00007ffff417b45f in g_main_context_iteration (context=0x7fffec005000,
103 may_block=may_block@entry=1) at ../glib/gmain.c:4015
104#43 0x00007ffff5f0dbee in QEventDispatcherGlib::processEvents (this=0x58ec30, flags=...)
105 at kernel/qeventdispatcher_glib.cpp:423
106#44 0x00007ffff5eb5b9b in QEventLoop::exec (this=this@entry=0x7fffffffbe50, flags=...,
107 flags@entry=...) at ../../include/QtCore/../../src/corelib/global/qflags.h:136
108#45 0x00007ffff5ebd972 in QCoreApplication::exec ()
109 at ../../include/QtCore/../../src/corelib/global/qflags.h:118
110#46 0x00007ffff648856c in QGuiApplication::exec () at kernel/qguiapplication.cpp:1866
111#47 0x00007ffff6d56c25 in QApplication::exec () at kernel/qapplication.cpp:2824
112#48 0x0000000000423cce in main (argc=<optimized out>, argv=<optimized out>)
113 at /home/nate/kde/src/spectacle/src/Main.cpp:166

Couldn't quite reproduce it the way you described, but indeed managed to get it to crash. It's fixed with D29014

ngraham added a subscriber: GB_2.Apr 20 2020, 5:00 PM

David asked for VDG to approve before this landed, which wasn't done.

Dude, I jumped through all the hoops for the past weeks. Also it got no further reply after I updated the screenshot almost two weeks ago so yes I assumed you guys had nothing else.

Sorry I didn't have the time to test it again. It's not just about the UI design itself, but also avoid obvious visual glitches, like this:

There are also numerous cases in other KCMs where the indicator gets cut off, positioned strangely, or isn't visible at all for a non-default setting:



How are we going to fix that? The diversity of user interfaces we have throughout KDE software makes me skeptical that any kind of auto-generated icon placement can ever work, let alone look good. Where will the icons go in list items? Grid views? And so on. To be completely honest, if nobody could come up with a good UI, it might be a sign that the feature itself needs to be re-thought. I remain unconvinced that this is the best way to show that there are changed settings. I think @GB_2's idea of displaying the original or previous state when hovering over the Defaults or Reset button made more sense. As is, I don't really understand who the target user is for this feature.

IMO this feature would have benefited from being outlined first in a Phabricator task and soliciting VDG feedback before coding began, so we didn't frustratingly go back and forth in the patches. Given the above visual regressions and broken behavior on multiple KCMs, I think this needs to be reverted and discussed and re-thought in a central location. Sorry. :(

bam added a subscriber: bam.Apr 20 2020, 5:38 PM
ervin added a comment.Apr 20 2020, 6:34 PM

You know it started as a proper painted indicator within the widget area, right? As such it couldn't have any of the issues you're pointing out now... Who pushed me to have them at distance I wonder? Right, was people from the VDG. So I find grand that then it goes all to revert because after weeks of pushing me to add more weird constraints then disappearing letting me alone trying to figure out where to go (and it was a large struggle at every step), the conclusion is "let's revert because VDG wasn't involved". There were technical reasons for the very first iteration and they had to be disregarded for design license.

Honestly I'm disgusted by the way it's been handled.

Yeah, I'm sorry about that.

If VDG people ask for something that's technically impossible, you've gotta push back on that. They often don't know what is and isn't possible, or reasonable. We've been trying to help VDG people be more technical so they don't propose impossible things, but it's not perfect. The whole process needs to be a push-and-pull compromise where the design people accept when a design isn't technically feasible, and the tech people faithfully implement the design without diverging too far from it due to minor technical limitations, or letting the design people push them into something impossible due to major technical limitations.

The basic problem with this feature is that I think we never did the initial design work to figure out who the target audience was, what their needs were, and why they would use and benefit from this feature. Even with an inline indicator like a glowing outline around the widget, we'd have the same problem that we do with the dot proposal in the sidebar view that it would be look like non-obvious visual noise to people. And with that, we'd lose the functionality of being able to revert individual settings. But is that needed? Who benefits from it? And so on. Such a complex and all-encompassing feature needs to have these kinds of questions answered first before implementation begins. We've found that Phabricator tasks are perfect for this, and we use them extensively to plan out work before coding begins for many projects.

I know we're all exhausted and frustrated at this point, but maybe we can do that so we can push this forward in a way that makes everyone happy in the end?

abetts added a subscriber: abetts.Apr 20 2020, 8:08 PM

My opinion on this patch from the beginning has been that it really doesn't add much more than was there before. Visually, it clutters the UI. The icon selected for it might also not be visually appealing or meaningful enough. I believe there should be a different approach to this request. However, I am also open to just dropping the idea completely.

Some ideas for better UX on this:

  1. Hover over the "Defaults" button and the UI shows highlighted controls and labels that have changed. If you want to return things to default, click the button.
  2. Color controls and labels differently once they have changed to a non-default state. From Breeze Blue to a lighter blue, for example. (This could be confusing to some users)
  3. Keyboard shortcut. Use a keyboard shortcut to return settings back to their default state. Control + Z comes to mind.
ndavis added a comment.EditedApr 20 2020, 8:29 PM

I'm sorry this happened. I was working with what I could see. For what it's worth, I would have accepted it if you said it could not be done well. I know I don't know as much about the technical side of this as you do. You have a right to disagree with me and tell me when I'm wrong.

Let's have the rest of the discussion in a central phab patch, which I probably should have pushed for at the start. I'll make one...