DevHeads.net

Review Request: Add KMessageWidget, an alternative to KMessageBox

Review request for kdelibs.

Summary
KMessageWidget is a new widget which can be considered as a less intrusive alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( <a href="http://community.kde.org/Sprints/UX2011/KMessageWidget" title="http://community.kde.org/Sprints/UX2011/KMessageWidget">http://community.kde.org/Sprints/UX2011/KMessageWidget</a> ).

The class documentation should make it clear how and when it can be used.

This widget could be used by:
- Browsers to implement their "remember password" widgets (Konqueror, Rekonq...)
- Forms to provide feedback on validation errors
- Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" message widget
- Nepomuk/Strigi KCM to indicate status of their services
- Gwenview to replace its custom save bar
- ...

I still have a few additions in mind for the API but it is already usable and since we are freezing I think it can be merged in master in its current state. I assume it will still be possible to extend the API a bit before kdelibs 4.7 freezes for good.

I also wrote an example program in the kdeexample repository ( <a href="https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget" title="https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget">https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=ag...</a> ), though I am wondering whether it shouldn't be moved in kdeui/tests as it is more a manual test program than an example.

Diffs
kdeui/CMakeLists.txt d1873d1
kdeui/widgets/kmessagewidget.h PRE-CREATION
kdeui/widgets/kmessagewidget.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/101249/diff" title="http://git.reviewboard.kde.org/r/101249/diff">http://git.reviewboard.kde.org/r/101249/diff</a>

Testing

Screenshots
Montage from kmessagewidgetdemo
<a href="http://git.reviewboard.kde.org/r/101249/s/141/" title="http://git.reviewboard.kde.org/r/101249/s/141/">http://git.reviewboard.kde.org/r/101249/s/141/</a>

Thanks,

Aurélien

Comments

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 04/29/2011 - 09:44

(Updated April 29, 2011, 1:44 p.m.)

Review request for kdelibs.

Changes
Applied all changes suggested by Jaros?aw.

Summary
KMessageWidget is a new widget which can be considered as a less intrusive alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( <a href="http://community.kde.org/Sprints/UX2011/KMessageWidget" title="http://community.kde.org/Sprints/UX2011/KMessageWidget">http://community.kde.org/Sprints/UX2011/KMessageWidget</a> ).

The class documentation should make it clear how and when it can be used.

This widget could be used by:
- Browsers to implement their "remember password" widgets (Konqueror, Rekonq...)
- Forms to provide feedback on validation errors
- Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" message widget
- Nepomuk/Strigi KCM to indicate status of their services
- Gwenview to replace its custom save bar
- ...

I still have a few additions in mind for the API but it is already usable and since we are freezing I think it can be merged in master in its current state. I assume it will still be possible to extend the API a bit before kdelibs 4.7 freezes for good.

I also wrote an example program in the kdeexample repository ( <a href="https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget" title="https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget">https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=ag...</a> ), though I am wondering whether it shouldn't be moved in kdeui/tests as it is more a manual test program than an example.

Diffs (updated)
kdeui/CMakeLists.txt d1873d1
kdeui/widgets/kmessagewidget.h PRE-CREATION
kdeui/widgets/kmessagewidget.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/101249/diff" title="http://git.reviewboard.kde.org/r/101249/diff">http://git.reviewboard.kde.org/r/101249/diff</a>

Testing

Screenshots
Montage from kmessagewidgetdemo
<a href="http://git.reviewboard.kde.org/r/101249/s/141/" title="http://git.reviewboard.kde.org/r/101249/s/141/">http://git.reviewboard.kde.org/r/101249/s/141/</a>

Thanks,

Aurélien

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 04/30/2011 - 09:10

(Updated April 30, 2011, 1:10 p.m.)

Review request for kdelibs.

Changes
Changed graphicEffectsLevel() check

Summary
KMessageWidget is a new widget which can be considered as a less intrusive alternative for KMessageBox. It was designed during KDE UX sprint 2011 ( <a href="http://community.kde.org/Sprints/UX2011/KMessageWidget" title="http://community.kde.org/Sprints/UX2011/KMessageWidget">http://community.kde.org/Sprints/UX2011/KMessageWidget</a> ).

The class documentation should make it clear how and when it can be used.

This widget could be used by:
- Browsers to implement their "remember password" widgets (Konqueror, Rekonq...)
- Forms to provide feedback on validation errors
- Bluedevil KCM to replace its custom "No Bluetooth adapter have been found" message widget
- Nepomuk/Strigi KCM to indicate status of their services
- Gwenview to replace its custom save bar
- ...

I still have a few additions in mind for the API but it is already usable and since we are freezing I think it can be merged in master in its current state. I assume it will still be possible to extend the API a bit before kdelibs 4.7 freezes for good.

I also wrote an example program in the kdeexample repository ( <a href="https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget" title="https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=agateau%2Fkmessagewidget">https://projects.kde.org/projects/kde/kdeexamples/repository/show?rev=ag...</a> ), though I am wondering whether it shouldn't be moved in kdeui/tests as it is more a manual test program than an example.

Diffs (updated)
kdeui/CMakeLists.txt d1873d154f4dde92c29f2e6dab1be70d49ddb55e
kdeui/widgets/kmessagewidget.h PRE-CREATION
kdeui/widgets/kmessagewidget.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/101249/diff" title="http://git.reviewboard.kde.org/r/101249/diff">http://git.reviewboard.kde.org/r/101249/diff</a>

Testing

Screenshots
Montage from kmessagewidgetdemo
<a href="http://git.reviewboard.kde.org/r/101249/s/141/" title="http://git.reviewboard.kde.org/r/101249/s/141/">http://git.reviewboard.kde.org/r/101249/s/141/</a>

Thanks,

Aurélien

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Albert Astals Cid at 05/01/2011 - 07:44

You seem to be missing the addition of the uppercase include in kdelibs/includes

kdeui/widgets/kmessagewidget.h
<http://git.reviewboard.kde.org/r/101249/#comment2569>

Do we really need this include here?

kdeui/widgets/kmessagewidget.h
<http://git.reviewboard.kde.org/r/101249/#comment2568>

I personally prefer all the variables having a name in the header too even if in this case it's pretty obvious what they do i think it won't hurt adding them either.

- Albert

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 05/03/2011 - 03:35

fixed

- Aurélien

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Aaron J. Seigo at 05/01/2011 - 08:39

i already fixed that :)

fixed

- Aaron J.

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Hugo Pereira Da... at 05/01/2011 - 06:17

Also, I wanted to add that Gtk already has a widget similar to your idea, called GtkInfoBar, and which is rendered by oxygen-gtk like in the screenshot below:

<a href="http://simplest-image-hosting.net/jpg-0-plasma-desktopou9644" title="http://simplest-image-hosting.net/jpg-0-plasma-desktopou9644">http://simplest-image-hosting.net/jpg-0-plasma-desktopou9644</a>

Now I must say it is not widely used in gtk land.

- Hugo

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 05/02/2011 - 13:40

Interesting, I was not aware of that widget.

- Aurélien

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Hugo Pereira Da... at 05/01/2011 - 06:14

Hello Aurelien,

Though I have nothing to say against the current design of your kmessagebox widget, something I miss in your implementation is the possibility for a widget style to override/replace your rendering. I can indeed imagine some styles (not oxygen though) for which your design might not integrate that well, notably "skulpture" for which all widgets have square corners, that won't match your nice rounded one.

There is a mechanism to "register" custom style primitives that widget styles may or may not support, and for your custom widget to test whether the style supports the new primitive or not, allowing it to fallback to its own rendering, when not supported. I think it would be nice for your widget (and other 'custom' widgets that you'd plan to implement) to use this mechanism, for better future integration.

kdeui/kdelibs/kcapacitybar implements this mechanism (the widget appears in the statusbar of dolphin to give the available space on a given device), and oxygen (as well as bespin) actually uses it to render the capacity bar as a regular "progress bar", instead of the default implementation (which you can see by using any style but oxygen, or bespin, e.g. plastique).

Tell me that you think, and also if needed I can help implementing if you agree with the above but are short of time to work on it.

Cheers,

Hugo

- Hugo

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Hugo Pereira Da... at 05/03/2011 - 03:55

I guess we should update the apidocs then. IMO the notion of progress of progressbar is only in its name. (and the fact that its value changes over time), not in the way it is actually rendered.

- Hugo

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 05/03/2011 - 03:37

Well... it "breaks" the documentation of the widget then, see: <a href="http://api.kde.org/4.5-api/kdelibs-apidocs/kdeui/html/classKCapacityBar.html" title="http://api.kde.org/4.5-api/kdelibs-apidocs/kdeui/html/classKCapacityBar.html">http://api.kde.org/4.5-api/kdelibs-apidocs/kdeui/html/classKCapacityBar....</a> (in the "Detailed Description" section)

- Aurélien

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Hugo Pereira Da... at 05/02/2011 - 14:27

Aurélien: "btw: I don't get the nice capacity bar in Dolphin anymore, just a progress bar, is it normal?"
Yes its normal. Its precisely what the "styling" mechanism allowed us to do. We (that is: Nuno, I, and some others) felt that the default implementation provided by Dolphin, although nice, had some polishing issues, and did not integrate well with Oxygen, so we re-implemented it in oxygen, and after some more discussion with Nuno, we figured that rather than implementing a new UI element with different rendering for just this purpose, we'd better just reuse the progress bar element, which which achieves just the same goal.

- Hugo

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 05/02/2011 - 13:33

@Hugo: Interesting, I didn't know about this mechanism. I will be quite busy for the upcoming two weeks so if you feel like implementing it, feel free to (btw: I don't get the nice capacity bar in Dolphin anymore, just a progress bar, is it normal?)

@Aaron: I was able to reproduce this bug for a while when the "content" widget was hidden away by moving it (0, content->height()). Strangely enough changing the code to move it to (0, -content->height()) fixed it for me.

- Aurélien

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Hugo Pereira Da... at 05/01/2011 - 09:12

Thanks Aaron.
Its now fixed (in a temporary way) in trunk.
Note: I've been planing to re-implement the label text transition for a looong time now, in a much more robust way, to avoid these kind of issues -that occur elsewhere in kde- ... But have not found time so far :(

- Hugo

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Aaron J. Seigo at 05/01/2011 - 08:41

note that this widget exposes a bug in oxygen style where when fading between two strings in a QLabel, the background is not updated. so when the text changes as well as the background color, the old background color remains. this is easily seen if you run kmessagewidgetdemo from kdeexamples/kmessagewidget/ and switch between the Information and Positive messages.

- Aaron J.

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Andrea Diamantini at 04/30/2011 - 06:12

I cannot properly review your code. Ill just say I like this new GUI object very much! many thanks for :)

- Andrea

On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Jaroslaw Staniek at 04/29/2011 - 15:10

Ship it!

- Jarosław

On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Jaroslaw Staniek at 04/29/2011 - 15:09

Ship it!

PS: the callouts I have implemented: <a href="http://simplest-image-hosting.net/jpg-0-plasma-desktophm4013" title="http://simplest-image-hosting.net/jpg-0-plasma-desktophm4013">http://simplest-image-hosting.net/jpg-0-plasma-desktophm4013</a>
It's BC with your API.

- Jarosław

On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 05/01/2011 - 12:14

Interesting! I am not sure however using a KMessageWidget is the best choice here: it looks like a modal choice, isn't it? At least I think the "No" button should be changed to "Cancel".

- Aurélien

On April 30, 2011, 1:10 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Jaroslaw Staniek at 04/29/2011 - 15:08

kdeui/widgets/kmessagewidget.cpp
<http://git.reviewboard.kde.org/r/101249/#comment2542>

please change '<' with '&', then the result is what we mean...

- Jarosław

On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Jaroslaw Staniek at 04/28/2011 - 16:06

Ship it!

I admit I reviewed the code extensively since I used it in Kexi already, extending by adding default action feature, callouts, and autodeletion. Good job!

Please look at these supplementary notes too - I'd like to start discussion:
There's possible problem with adding new widgets in so matured API as KDElibs 4.7: I do not know how we'll deal with the problem of supporting KDE 4.6 apps that require this new widget (without forking). As I said on kde-core-devel, ideally new widgets would land in extra library and could be backported. Also this widget has almost nothing to do with KDElibs-sepcific features and some peopel would _love_ to use it in non-KDE code too. Again, without forking.

kdeui/widgets/kmessagewidget.h
<http://git.reviewboard.kde.org/r/101249/#comment2529>

How about having Q_Properties ?

kdeui/widgets/kmessagewidget.h
<http://git.reviewboard.kde.org/r/101249/#comment2528>

It would be good to add ctor KMessageWidget(const QString* text, parent) ctor too for convenience, it's frequent case

kdeui/widgets/kmessagewidget.h
<http://git.reviewboard.kde.org/r/101249/#comment2527>

setCloseButtonVisible() would be better than verb show

Also isCloseButtonVisible() setter needed for completness

Also it would be good to make setCloseButtonVisible() a slot since other property (wordWrap) has setter as slot

kdeui/widgets/kmessagewidget.cpp
<http://git.reviewboard.kde.org/r/101249/#comment2530>

Do we need this attribute? labelText->text() has the same information.

kdeui/widgets/kmessagewidget.cpp
<http://git.reviewboard.kde.org/r/101249/#comment2533>

I suggest adding
textLabel->setTextInteractionFlags(Qt::TextBrowserInteraction);

we use such flag in KMessageBox too, this enables links and text selection, useful when error copying texts by users

kdeui/widgets/kmessagewidget.cpp
<http://git.reviewboard.kde.org/r/101249/#comment2531>

Please change to SimpleAnimationEffects, since in other places this kind of animations are allowed for SimpleAnimationEffects level, e.g.: KFadeWidgetEffect.

- Jarosław

On April 28, 2011, 3:31 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By =?UTF-8?B?QXVyw... at 04/29/2011 - 09:58

autodeletion and default action features make sense. Not sure what you mean with callouts though.

Regarding putting this in a separate library: it uses quite a few KDE features: KIconLoader, KColorScheme, KStandardAction... it is not really ready to be used from a Qt-only application.

I thought a lot about your idea of using separate libraries for classes added in future version of kdelibs. One of the drawbacks I foresee is the following:
- Assuming class A is in libkdeui
- A new class, B, is created and added to libkdeui4.7
- If it makes sense to use B in A, then libkdeui needs to depend on libkdeui4.7...

I think it would be simpler to document a clean way for application developers to backport new classes they are interested in. This can be done in a way which automatically avoids building the copied classes if the application is built against the latest version of kdelibs, ensuring the backports "disable themselves" automatically.

- Aurélien

On April 29, 2011, 1:44 p.m., Aurélien Gâteau wrote:

Re: Review Request: Add KMessageWidget, an alternative to KMessa

By Jaroslaw Staniek at 04/28/2011 - 16:12

BTW, links to my extensions:
<a href="http://quickgit.kde.org/?p=calligra.git&amp;a=blob&amp;hb=60e7ff67fee20723a4a318339d982c85253307a5&amp;f=kexi/kexiutils/kmessagewidget.h" title="http://quickgit.kde.org/?p=calligra.git&amp;a=blob&amp;hb=60e7ff67fee20723a4a318339d982c85253307a5&amp;f=kexi/kexiutils/kmessagewidget.h">http://quickgit.kde.org/?p=calligra.git&amp;a=blob&amp;hb=60e7ff67fee20723a4a318...</a>
<a href="http://quickgit.kde.org/?p=calligra.git&amp;a=blob&amp;h=67d11d15bd8c7df894ea7b048aeb9306efb7a1ab&amp;hb=60e7ff67fee20723a4a318339d982c85253307a5&amp;f=kexi/kexiutils/kmessagewidget.cpp" title="http://quickgit.kde.org/?p=calligra.git&amp;a=blob&amp;h=67d11d15bd8c7df894ea7b048aeb9306efb7a1ab&amp;hb=60e7ff67fee20723a4a318339d982c85253307a5&amp;f=kexi/kexiutils/kmessagewidget.cpp">http://quickgit.kde.org/?p=calligra.git&amp;a=blob&amp;h=67d11d15bd8c7df894ea7b0...</a>

- Jarosław

On April 28, 2011, 3:31 p.m., Aurélien Gâteau wrote: