> 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. > > Oswald Buddenhagen wrote: > 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 > > Konstantinos Smanis wrote: > 1) I am not quite following you here. Would it be fine to pick a fixed > separator and escape it when necessary? > 2) Submenus are valid selections in Grub2. > 3) This happens only when committing Grub2/Burg (by means of an external > command), does this qualify as an API break? > > Oswald Buddenhagen wrote: > 1) yes. though i don't think escaping is worth it if you choose a > sufficiently complex separator (like " >> " or " // "). whatever. > 2) and what exactly is supposed to happen then? why would it be useful? > 3) yes, it does, because it violates the goal that ksmserver and kdm from > different versions can talk with each other. > > Konstantinos Smanis wrote: > 1) Personally I find escaping "cleaner". You never know what a user can > input and break stuff. > 2) As far as I know no entry is booted and Grub2 lingers on this submenu > until the user manually makes a selection. Don't ask me the whys behind > Grub's implementation, it's just valid to specify submenus. Anyway, with the > current implementation submenus aren't selectable, but even if they were > nothing would break. > 3) ksmserver uses strings anyway, not stringified integers. Only the > communication between kdm and the grub2-reboot exe is affected and it doesn't > break (grub2-reboot always accepted either integers or strings - it will > accept anything at all actually).
1) of course. otoh, it adds complexity for an utter corner case. just rejecting the input is perfectly fine in such a case. just make use it is really uncommon by choosing a sufficiently complex separator (which still looks natural if presented literally). but then, if you implement separator normalization anyway, separator escaping won't add much additional complexity. 2) an old ksmserver *would* allow selecting this. i don't think it's desirable to allow this - it produces an inconsistent behavior with leaf selections, and it's not particularly useful. 3) oh. right. i even did that on purpose. a long time ago. - 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 > >
