Review Request: Support for GRUB2 submenus

[Oswald Buddenhagen]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105563/#review17015
-----------------------------------------------------------


looking excellent now. just some style issues left.

please write a somewhat more compact commit message - basically, bullet points about everything which is not self-evident when reading the patch.


kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13358>

    rename to "len".



kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13359>

    you are using strlen of before and after several times, including in loops. it's probably a good idea to calculate beforeLen and afterLen early.



kdm/backend/util.c
<http://git.reviewboard.kde.org/r/105563/#comment13360>

    this could also use strCatL with afterLen



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13361>

    this is not c - it's usually preferred to declare variables close to their first use. that would mean before line 492 for action, and on line 485 for label.



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13364>

    an alternative way to write that would be
    
      rebootOptions.at(i).mid(n < 0 ? 0 : n + sep.length())
    
    it has less redundancy, so i think it's nicer (it's not notably less efficient, as mid() detects the no-op case).



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13363>

    remove the braces (qt style here, not kdelibs).



ksmserver/shutdowndlg.cpp
<http://git.reviewboard.kde.org/r/105563/#comment13362>

    i'd swap this with the next line. makes the data flow more logical.


- Oswald Buddenhagen


On Aug. 6, 2012, 6:29 p.m., Konstantinos Smanis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2012, 6:29 p.m.)
> 
> 
> Review request for KDE Runtime and Oswald Buddenhagen.
> 
> 
> Description
> -------
> 
> Recent versions of GRUB2 introduce submenus which allow for nesting levels to appear (instead of the flat list in the past).
> This patch consists of two parts: the parsing part in KDM (bootman.c) and creating a menu structure from the parsed list in ksmserver (shutdowndlg.*)
> The parsing part produces a list like this:
> 
> Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4
> Advanced options for Gentoo GNU/Linux>Gentoo GNU/Linux, with Linux 3.4.4 (recovery mode)
> Windows 7 (loader) (on /dev/sda2)
> ???? ?????????
> 
> which is then converted into the menu structure. These full identifiers can be properly used with `grub2-reboot`.
> 
> More info about submenus: http://ubuntuforums.org/showthread.php?p=10720316
> Also check the related bug.
> 
> The parsing part of the patch can be applied in the KDE/4.9 and master branches as well (tested and working in KDE 4.8.95 and 4.9.x). ksmserver has migrated to QML since then however, and the menu structure has to wait. Currently it looks like this: http://i50.tinypic.com/96bw35.png
> Related ML-discussion: http://lists.kde.org/?l=kde-core-devel&m=134160279511422&w=2
> Update: There is a proper fix now for KDE/4.9 and master: https://git.reviewboard.kde.org/r/105568/
> 
> 
> This addresses bug 297209.
>     http://bugs.kde.org/show_bug.cgi?id=297209
> 
> 
> Diffs
> -----
> 
>   kdm/backend/bootman.c 8b834d2 
>   kdm/backend/ctrl.c 5d219fe 
>   kdm/backend/dm.h 13e7b45 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.cpp a09a1a7 
> 
> Diff: http://git.reviewboard.kde.org/r/105563/diff/
> 
> 
> Testing
> -------
> 
> Works with the menu file produced in my system with `grub2-mkconfig`.
> Also works with a custom menu file I made (as shown in the second screenshot).
> 
> 
> Screenshots
> -----------
> 
> Distribution's stock menu file
>   http://git.reviewboard.kde.org/r/105563/s/633/
> A custom menu file for testing
>   http://git.reviewboard.kde.org/r/105563/s/634/
> 
> 
> Thanks,
> 
> Konstantinos Smanis
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120807/e309f793/attachment.htm>