Review Request: Support for GRUB2 submenus

Wed Aug 1 09:46:47 BST 2012

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 231
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line231>
> >
> >     this function must come with a huge warning that strlen(after) must be at most strlen(before) - otherwise you overflow the buffer.

Oh yeah, my first implementation used a simple char for "after" which was fine. I screwed it when generalizing it.

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 239
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line239>
> >
> >     this implementation is very clever, but i wonder whether it wouldn't be better to do something more conventional, like a loop of strstr() and memcpy().

Sure, I just made that up. If you want to go for clarity I can try a strstr implementation.

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 275
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line275>
> >
> >     i think this loop should just reduce len

Hmm, I remember that originally I was reducing len, then switched to i. If I can't remember a proper explanation I will switch back.

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 281
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line281>
> >
> >     if a closing brace can appear only alone, this line should become an else-if. if it can come after another statement, we need to think some more.

What do you mean here? A single if-else would require ANDing the external condition (line ending in '}').

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/bootman.c, line 472
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75565#file75565line472>
> >
> >     now you went kinda overboard - you are *both* using a complex normalized separator *and* announcing it. i think that given the former you can just remove the latter. i meant to imply that in my "ok, this all sucks, let's do it differently"-comment, but i clearly failed.

Should I hard-code it in ksmserver too then?

> On Aug. 1, 2012, 6:25 a.m., Oswald Buddenhagen wrote:
> > ksmserver/shutdowndlg.cpp, line 485
> > <http://git.reviewboard.kde.org/r/105563/diff/11/?file=75571#file75571line485>
> >
> >     why do you do that? the non-nested case should be just the simple solution of the general nested case.

I noticed that my menuentry-case code kinda duplicates the old code but could you please elaborate a little more on this? I am not sure I get how you want the implementation. If-nesting for submenus and simple actions outside of it?

- Konstantinos

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

On July 31, 2012, 5:54 p.m., Konstantinos Smanis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105563/
> -----------------------------------------------------------
> 
> (Updated July 31, 2012, 5:54 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/dm.c e0f1366 
>   kdm/backend/util.c 6cd93ef 
>   ksmserver/shutdowndlg.h e5f0942 
>   ksmserver/shutdowndlg.cpp a09a1a7 
>   libs/kworkspace/kdisplaymanager.h 76f25a7 
>   libs/kworkspace/kdisplaymanager.cpp 28fabfc 
> 
> 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/20120801/095f1473/attachment.htm>