DevHeads.net

Review Request: Add submenu support to QML shutdown dialog.

Review request for KDE Runtime and Konstantinos Smanis.

Description
Add support to show submenus in the new QML shutdown dialog. I think this patch can be improved, the GUI too. When I have more time I will go back to improve it, until there you can send suggestions.

This addresses bug 297209.
<a href="http://bugs.kde.org/show_bug.cgi?id=297209" title="http://bugs.kde.org/show_bug.cgi?id=297209">http://bugs.kde.org/show_bug.cgi?id=297209</a>

Diffs
ksmserver/themes/default/main.qml 7e78761

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

Testing
Submenus are created and it emits rebootRequested2 signal with the correct index.

Thanks,

Lamarque Vieira Souza

Comments

Re: Review Request: Add submenu support to QML shutdown dialog.

By Lamarque V. Souza at 07/13/2012 - 22:46

(Updated July 14, 2012, 2:46 a.m.)

Review request for KDE Runtime and Konstantinos Smanis.

Changes
Update description.

Description (updated)
Add support to show submenus in the new QML shutdown dialog. I think this patch can be improved, the GUI too. When I have more time I will go back to improve it, until there you can send suggestions.

The patch assumes rebootOptions now contains strings like:

entry1
submenu1 > subentry 1.1
submenu1 > subentry 1.2
submenu2 > subentry 2.1
submenu2 > subsubmenu 1 > subsubentry 2.1.1
entry2

The character '>' is the separator for submenus.

This addresses bug 297209.
<a href="http://bugs.kde.org/show_bug.cgi?id=297209" title="http://bugs.kde.org/show_bug.cgi?id=297209">http://bugs.kde.org/show_bug.cgi?id=297209</a>

Diffs
ksmserver/themes/default/main.qml 7e78761

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

Testing
Submenus are created and it emits rebootRequested2 signal with the correct index.

Thanks,

Lamarque Vieira Souza

Re: Review Request: Add submenu support to QML shutdown dialog.

By Lamarque V. Souza at 07/14/2012 - 01:41

(Updated July 14, 2012, 5:41 a.m.)

Review request for KDE Runtime and Konstantinos Smanis.

Changes
. align menu entries to the left instead of center.
. add hover effect.
. add icon to the menu entry to indicate that it contains a submenu.
. close all submenus when one entry is clicked.
. add screenshot

Description
Add support to show submenus in the new QML shutdown dialog. I think this patch can be improved, the GUI too. When I have more time I will go back to improve it, until there you can send suggestions.

The patch assumes rebootOptions now contains strings like:

entry1
submenu1 > subentry 1.1
submenu1 > subentry 1.2
submenu2 > subentry 2.1
submenu2 > subsubmenu 1 > subsubentry 2.1.1
entry2

The character '>' is the separator for submenus.

This addresses bug 297209.
<a href="http://bugs.kde.org/show_bug.cgi?id=297209" title="http://bugs.kde.org/show_bug.cgi?id=297209">http://bugs.kde.org/show_bug.cgi?id=297209</a>

Diffs (updated)
ksmserver/themes/default/ContextMenu.qml 6f2f1fd
ksmserver/themes/default/MenuItem.qml 74bb03f
ksmserver/themes/default/main.qml 7e78761

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

Testing
Submenus are created and it emits rebootRequested2 signal with the correct index.

Screenshots (updated)

<a href="http://git.reviewboard.kde.org/r/105568/s/635/" title="http://git.reviewboard.kde.org/r/105568/s/635/">http://git.reviewboard.kde.org/r/105568/s/635/</a>

Thanks,

Lamarque Vieira Souza

Re: Review Request: Add submenu support to QML shutdown dialog.

By Lamarque V. Souza at 07/14/2012 - 01:48

(Updated July 14, 2012, 5:48 a.m.)

Review request for KDE Runtime and Konstantinos Smanis.

Changes
remove extra white spaces.

Description
Add support to show submenus in the new QML shutdown dialog. I think this patch can be improved, the GUI too. When I have more time I will go back to improve it, until there you can send suggestions.

The patch assumes rebootOptions now contains strings like:

entry1
submenu1 > subentry 1.1
submenu1 > subentry 1.2
submenu2 > subentry 2.1
submenu2 > subsubmenu 1 > subsubentry 2.1.1
entry2

The character '>' is the separator for submenus.

This addresses bug 297209.
<a href="http://bugs.kde.org/show_bug.cgi?id=297209" title="http://bugs.kde.org/show_bug.cgi?id=297209">http://bugs.kde.org/show_bug.cgi?id=297209</a>

Diffs (updated)
ksmserver/themes/default/ContextMenu.qml 6f2f1fd
ksmserver/themes/default/MenuItem.qml 74bb03f
ksmserver/themes/default/main.qml 7e78761

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

Testing
Submenus are created and it emits rebootRequested2 signal with the correct index.

Screenshots

<a href="http://git.reviewboard.kde.org/r/105568/s/635/" title="http://git.reviewboard.kde.org/r/105568/s/635/">http://git.reviewboard.kde.org/r/105568/s/635/</a>

Thanks,

Lamarque Vieira Souza

Re: Review Request: Add submenu support to QML shutdown dialog.

By Lamarque V. Souza at 07/14/2012 - 14:53

(Updated July 14, 2012, 6:53 p.m.)

Review request for KDE Runtime and Konstantinos Smanis.

Changes
. do not treat ampersand as accelerator.
. adjust menu entries's width.
. close all menus when pressing ESC.

Description
Add support to show submenus in the new QML shutdown dialog. I think this patch can be improved, the GUI too. When I have more time I will go back to improve it, until there you can send suggestions.

The patch assumes rebootOptions now contains strings like:

entry1
submenu1 > subentry 1.1
submenu1 > subentry 1.2
submenu2 > subentry 2.1
submenu2 > subsubmenu 1 > subsubentry 2.1.1
entry2

The character '>' is the separator for submenus.

This addresses bug 297209.
<a href="http://bugs.kde.org/show_bug.cgi?id=297209" title="http://bugs.kde.org/show_bug.cgi?id=297209">http://bugs.kde.org/show_bug.cgi?id=297209</a>

Diffs (updated)
ksmserver/themes/default/ContextMenu.qml 6f2f1fd
ksmserver/themes/default/MenuItem.qml 74bb03f
ksmserver/themes/default/main.qml 7e78761

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

Testing
Submenus are created and it emits rebootRequested2 signal with the correct index.

Screenshots

<a href="http://git.reviewboard.kde.org/r/105568/s/635/" title="http://git.reviewboard.kde.org/r/105568/s/635/">http://git.reviewboard.kde.org/r/105568/s/635/</a>

Thanks,

Lamarque Vieira Souza

Re: Review Request: Add submenu support to QML shutdown dialog.

By Konstantinos Smanis at 07/17/2012 - 14:19

I forgot to mention that the submenus should only be created if Grub2 or Burg is the selected Boot Manager in KDM. Other bootloaders (grub, lilo) should still be able to use '>' in the menu titles without creating submenus. See the ksmserver part of the other patch I am submitting for branch KDE/4.8 (<a href="https://git.reviewboard.kde.org/r/105563/" title="https://git.reviewboard.kde.org/r/105563/">https://git.reviewboard.kde.org/r/105563/</a>).

- Konstantinos Smanis

On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Commit Hook at 07/17/2012 - 13:40

This review has been submitted with commit 7884098c74837db178a01c042cd187d37fd254b5 by Lamarque V. Souza to branch master.

- Commit Hook

On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Commit Hook at 07/17/2012 - 13:40

This review has been submitted with commit 0e325a61a57ff85f15bd3f84e57b5800ef32de07 by Lamarque V. Souza to branch KDE/4.9.

- Commit Hook

On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Konstantinos Smanis at 07/14/2012 - 15:36

The issues were resolved; much better now.

- Konstantinos Smanis

On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Konstantinos Smanis at 07/14/2012 - 08:18

My QML knowledge is too limited to review your code, but I tested your patch and have the following remarks to make:

1) There is some flickering when hovering a menu item. Dunno if it has to do with my theme (tried Oxygen + Caledonia).
2) There is no proper '&' escaping. Ampersands are falsely treated as keyboard accelerators (the old code provisioned for this). A double ampersand is shown as a single.
3) When the menu item's text is small (~4-5 characters), the arrow is shown very close to the text.

The core functionality seems to be fine, I onlyn noticed the above GUI issues.

- Konstantinos Smanis

On July 14, 2012, 5:48 a.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Konstantinos Smanis at 07/14/2012 - 15:33

Actually the flickering occurs even without the patch (but it's not that noticeable). It doesn't have to do with this specific patch.

- Konstantinos

On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Lamarque V. Souza at 07/14/2012 - 14:54

I do not see any flickering here. However, I disable several Oxygen effects here to improve performance, maybe one of them is causing your problem.

- Lamarque Vieira

On July 14, 2012, 6:53 p.m., Lamarque Vieira Souza wrote:

Re: Review Request: Add submenu support to QML shutdown dialog.

By Konstantinos Smanis at 07/14/2012 - 08:23

This is how it looks like over here with Oxygen: <a href="http://i46.tinypic.com/2m47rrm.png" title="http://i46.tinypic.com/2m47rrm.png">http://i46.tinypic.com/2m47rrm.png</a>

- Konstantinos

On July 14, 2012, 5:48 a.m., Lamarque Vieira Souza wrote: