<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105563/">http://git.reviewboard.kde.org/r/105563/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 17th, 2012, 10:23 p.m., <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/105563/diff/2/?file=73177#file73177line477" style="color: black; font-weight: bold; text-decoration: underline;">ksmserver/shutdowndlg.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KSMShutdownDlg::KSMShutdownDlg( QWidget* parent,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">477</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">bootManager</span> <span class="o">=</span> <span class="n">KConfig</span><span class="p">(</span><span class="n">KDE_CONFDIR</span> <span class="s">"/kdm/kdmrc"</span><span class="p">,</span> <span class="n">KConfig</span><span class="o">::</span><span class="n">SimpleConfig</span><span class="p">).</span><span class="n">group</span><span class="p">(</span><span class="s">"Shutdown"</span><span class="p">).</span><span class="n">readEntry</span><span class="p">(</span><span class="s">"BootManager"</span><span class="p">,</span> <span class="s">"None"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
<p>On July 18th, 2012, 12:35 a.m., <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I didn't quite get you here.</pre>
</blockquote>
<p>On July 18th, 2012, 1:49 a.m., <b>Lamarque Vieira Souza</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On July 18th, 2012, 7:58 a.m., <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah, I got it now. Yeah, Grub2 uses '>' without whitespaces around it (Burg follows suit most of the times).</pre>
</blockquote>
<p>On July 29th, 2012, 12:38 p.m., <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On July 29th, 2012, 12:53 p.m., <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On July 29th, 2012, 1:06 p.m., <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">the point of the above code snippet is precisely to demonstrate how kdm would publish the separator</pre>
</blockquote>
<p>On July 29th, 2012, 1:29 p.m., <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I got that, but what separator will kdm choose? It has to be something unique amongst all boot managers.</pre>
</blockquote>
<p>On July 29th, 2012, 1:34 p.m., <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</blockquote>
<p>On July 29th, 2012, 1:41 p.m., <b>Konstantinos Smanis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What if there is a Grub/Lilo title with '>' inside it? It will erroneously be resolved as submenu.</pre>
</blockquote>
<p>On July 29th, 2012, 2:01 p.m., <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
<br />
<p>- Konstantinos</p>
<br />
<p>On July 29th, 2012, 11:58 a.m., Konstantinos Smanis wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Runtime and Oswald Buddenhagen.</div>
<div>By Konstantinos Smanis.</div>
<p style="color: grey;"><i>Updated July 29, 2012, 11:58 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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/</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=297209">297209</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdm/backend/bootman.c <span style="color: grey">(8b834d2)</span></li>
<li>ksmserver/shutdowndlg.h <span style="color: grey">(e5f0942)</span></li>
<li>ksmserver/shutdowndlg.cpp <span style="color: grey">(a09a1a7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105563/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/105563/s/633/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/07/13/snapshot5_400x100.png" style="border: 1px black solid;" alt="Distribution's stock menu file" /></a>
<a href="http://git.reviewboard.kde.org/r/105563/s/634/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/07/13/snapshot6_400x100.png" style="border: 1px black solid;" alt="A custom menu file for testing" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>