> On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote: > > kdm/backend/bootman.c, line 282 > > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73175#file73175line282> > > > > as you are putting the submenus into the selection list anyway, "menus" > > needs to be only a stack of integers (and you can just allocate a > > fixed-size array for that - nobody is going to nest beyond 3 (does grub > > even support nesting? i didn't gather that impression). > > Konstantinos Smanis wrote: > Grub2 now supports nesting (dunno up to which level but I have tested > with 3 levels and it worked). Distributions create only one nesting level to > my knowledge but that doesn't mean a user can't have custom generation > scripts. > > Konstantinos Smanis wrote: > Another reason we can't use integers is because the submenus' names are > prepended to nested menuentries and this list will be shown to the user. We > could use integers as internal representation but imho that would be unfair > memory-to-code-clarity tradeoff. > > Oswald Buddenhagen wrote: > huh? the "menus" array is internal, so any replacement for it should be > too by definition. > > Konstantinos Smanis wrote: > The contents of the "menus" array appear in the opts array which in turn > is public. If you have > > submenu 'foo' { > menuentry 'bar' { > } > } > > This will be sent as 'foo>bar' to ksmserver. Providing '0>bar' to > ksmserver would be quite a hassle (it is valid Grub2 notation but of little > use to the user). > > Oswald Buddenhagen wrote: > and who exactly said that you are supposed to put stringified integers > into opts? > > Konstantinos Smanis wrote: > So you suggest that we maintain a stack of integers pointing to the > corresponding titles in the "opts" array? Will this be fixed size? If so, > what size? If not, should I implement a dynamic stack? > > Oswald Buddenhagen wrote: > all the answers are in the very first comment > > Konstantinos Smanis wrote: > A size of 3 seems too small to me. Size increases for each submenu > statement in the configuration. Most distros may use only 1 submenu but 3 > will fail easily for other users.
it is entirely absurd that somebody would create a boot menu structure with more than three levels. but whatever - make it five. just don't forget the overflow check. > On July 17, 2012, 10:23 p.m., Oswald Buddenhagen wrote: > > ksmserver/shutdowndlg.cpp, line 477 > > <http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477> > > > > no way. the backend should directly communicate the hierarchy > > separator. also to ksmserver (this is just the start): > > --- a/kdm/backend/ctrl.c > > +++ b/kdm/backend/ctrl.c > > @@ -769,9 +769,10 @@ processCtrl(const char *string, int len, int fd, > > struct display *d) > > } else if (!strcmp(ar[0], "listbootoptions")) { > > char **opts; > > + const char *sep; > > int def, cur, i, j; > > > > if (ar[1]) > > goto exce; > > - switch (getBootOptions(&opts, &def, &cur)) { > > + switch (getBootOptions(&opts, &def, &cur, &sep)) { > > case BO_NOMAN: > > fLog(d, fd, "notsup", "boot options unavailable"); > > @@ -796,5 +797,5 @@ processCtrl(const char *string, int len, int fd, > > struct display *d) > > } > > freeStrArr(opts); > > - writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\n", def, cur)); > > + writer(fd, cbuf, sprintf(cbuf, "\t%d\t%d\t%s\n", def, cur, > > sep)); > > goto bust; > > } else if (d) { > > ossi > > Konstantinos Smanis wrote: > I didn't quite get you here. > > Lamarque Vieira Souza wrote: > He wants that kdm passes the separator char to ksmserver instead of > hardcoding it to '>'. The code above sends the boot options to ksmserver I > guess, now including the separator char as parameter. In your patch both > grub2 and burg support submenus, do both use '>' as separator? If not then we > will need to pass the separator char to ksmserver. > > Konstantinos Smanis wrote: > Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it > (Burg follows suit most of the times). > > Oswald Buddenhagen wrote: > it's irrelevant what grub or burg use. the interface is supposed to be > boot manager agnostic, and should be even re-implementable by another login > manger. the mere fact that you have ksmserver read kdm's config file should > ring all alarm bells. > > Konstantinos Smanis wrote: > And what would be the separator? It has to be a sequence of characters > that will never be found in a menu entry title in any boot manager > configuration. > > Oswald Buddenhagen wrote: > the point of the above code snippet is precisely to demonstrate how kdm > would publish the separator > > Konstantinos Smanis wrote: > I got that, but what separator will kdm choose? It has to be something > unique amongst all boot managers. > > Oswald Buddenhagen wrote: > as the boot manager support is hard-coded in kdm, you can just continue > to use ">" for simplicity. but the ksmserver side needs to be able to deal > with arbitrary strings (including multiple chars). > > Konstantinos Smanis wrote: > What if there is a Grub/Lilo title with '>' inside it? It will > erroneously be resolved as submenu. the choice of separator is bound to the particular boot manager. consequently the problem does not exist for lilo. i don't know how to tell apart grub with and without submenus, but this problem is not specific to my request. anyway, i more and more dislike numerous aspects of your approach: - the fact that you pass grub's separator verbatim to the gui will make it look rather bad with older guis (e.g., a customized kde 4.9 build launched from a system kde 4.10). normalizing to a fixed separator would look nicer. this is orthogonal to what you pass to grub - you can have a second list which contains integer indices. this also makes the discussion about publishing the separator moot, as it would be fixed. literal appearances of the separator could be either escaped or just rejected. - the fact that you pass submenus will allow an older gui to make an invalid selection. the gui should infer the submenus from the leaf nodes instead - the fact that you change the interface to use strings instead of stringified integers is an api break and thus not allowed - Oswald ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105563/#review16042 ----------------------------------------------------------- On July 29, 2012, 11:58 a.m., Konstantinos Smanis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105563/ > ----------------------------------------------------------- > > (Updated July 29, 2012, 11:58 a.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 > ksmserver/shutdowndlg.h e5f0942 > 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 > >
