Merge lp://staging/~csurbhi/ubuntu/natty/plymouth/plymouth.bug-fix566818 into lp://staging/ubuntu/natty/plymouth

Following are some details of the patch:
1) The patch prints the entire prompt the first time (i.e when number of bullets to be printed is zero). It prints only the last line when the user presses a key (i.e bullets > 0)

2) The patch removes the independent printing of ':' as this is included in the prompt. No other plugin prints the ':' separately. So it makes sense to remove it from here and add it to the default prompt or expect it in the user defined prompt (just like in other plugins)

Hi Surbhi,

+ * Returns the position of the last newline character.
+ * If the newline character is at the end of the string,
+ * it returns the previous newline character in the string
+ * if any.

Why backing up to the previous newline character? This seems to just result in additional special handling down the line when write_on_views() is called, and is inconsistent anyway with the current handling as it means the prompt will lose the final newline, and the bullets will be drawn on the same line as the prompt instead of on the line after it. (Do you have examples of such a prompt today that ends in a newline?) It wouldn't be good form to end the prompt with a newline, but I don't think the details plugin is the place to correct for this.

+ write_on_views (plugin,
+ prompt,
+ prompt[len-1] == '\n' ? len -1 : len);
+ } else
+ write_on_views (plugin,
+ prompt,
+ strlen (prompt));
+ }

If final newlines don't need special treatment (and I think they shouldn't), then we can reduce the code duplication here as well.

   else
     write_on_views (plugin,
- "Password",
- strlen ("Password"));
-
- write_on_views (plugin, ":", strlen (":"));
+ "Password:",
+ strlen ("Password:"));

This is a fix for the separate issue of a doubled ':' in the prompt, right? It would be best if this were called out separately - perhaps in a dep3 header in debian/patches/ubuntu_details.patch that explains what the patch is for. (No need to split this as a separate commit or go into great detail in the changelog, but it should be documented somewhere so that it's clear this isn't an accidental change.)

Hi Steve,

Thanks a lot for reviewing the code. Here is what I thought:

1) If the prompt accidentally ends in a "\n" (which ideally it should not and I dont have an example thankfully to show this :), but if it does anyway), then the repetition of the prompt wont be fixed. Every other plugin can handle the case where there is a "\n" at the end except for the details plugin. So not printing the last character -'\n' in this special (probably a never should happen) is to prevent the case where a prompt does not appear and bullets get repeated. This will be very difficult for someone to debug - very non intuitive:
Eg: prompt: "Enter \n passphrase \n"

What gets printed is:
Enter
passphrase:
* (on next keystroke this is then cleared and \n is printed)
** (on next keystroke this is then cleared and \n is printed)
***
and so on!

Removing the last '\n' would rather print:
Enter
passphrase: ***

Please do let me know what you think!
I will make a separate patch of the ':'. Indeed that will be better to comprehend.

On Tue, Apr 19, 2011 at 08:08:28AM -0000, Surbhi Palande wrote:

> Thanks a lot for reviewing the code. Here is what I thought:

> 1) If the prompt accidentally ends in a "\n" (which ideally it should not
> and I dont have an example thankfully to show this :), but if it does
> anyway), then the repetition of the prompt wont be fixed. Every other
> plugin can handle the case where there is a "\n" at the end except for the
> details plugin. So not printing the last character -'\n' in this special
> (probably a never should happen) is to prevent the case where a prompt
> does not appear and bullets get repeated. This will be very difficult for
> someone to debug - very non intuitive: Eg: prompt: "Enter \n passphrase
> \n"

> What gets printed is:
> Enter
> passphrase:
> * (on next keystroke this is then cleared and \n is printed)
> ** (on next keystroke this is then cleared and \n is printed)
> ***
> and so on!

> Removing the last '\n' would rather print:
> Enter
> passphrase: ***

> Please do let me know what you think!

My understanding of the code is that the prompt is only shortened when
bullets > 0. So since we start out with 0 bullets, first you would have
this:

Enter
passphrase
_

Then because we're printing the part of the prompt between the
second-to-last and last newlines, typing a letter would refresh this to:

Enter
passphrase
passphrase *_

If we always cut everything up to and including the last \n, we instead get
an empty prompt when redrawing. And that's ok; given a confusingly
newline-terminated prompt, that's still probably better, as it gives us
this:

Enter
passphrase
***_

So I stand by my recommendation to not special-case a prompt that ends in a
newline.

> I will make a separate patch of the ':'. Indeed that will be better to
> comprehend.

Ok!

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

Yes, if we keep the last_nl pointing to the second to last '\n' as it does right now then yes, thats correct: the second line will be repeated over and over again - but then that has a simple fix too - by changing the prompt without the last '\n'.

Ok! I will rewrite the patch and upload here! Thanks!

So: the shorten_prompt() needs to be kept the way it is right now: by returning the second to the last '\n' in case the '\n' is the last character - to get a non empty prompt. I was assuming that needed to be changed to return the last '\n' (in which case we will get an empty string and a '\n' for every key stroke). Roughly speaking, I will change the part:
write_on_views (plugin,
+ prompt,
+ prompt[len-1] == '\n' ? len -1 : len);

to

write_on_views (plugin,
+ prompt,
+ len);

Hi Steve,

I have pushed the changes requested. Please do have a look! Thanks!

Regards,
Surbhi.

@@ -363,16 +384,16 @@
                     strlen (CLEAR_LINE_SEQUENCE));
   plugin->state = PLY_BOOT_SPLASH_DISPLAY_PASSWORD_ENTRY;

- if (prompt)
+ if (prompt) {
+ shorten_prompt(&prompt);
     write_on_views (plugin,
                     prompt,
                     strlen (prompt));
+ }
   else

This seems to drop the special handling of the bullets == 0 case. Sorry if my comments were unclear, I do believe that we still need to handle that case specially. Testing with the patch as-is, I never see the first line of the cryptsetup prompt.

Steve, I have added a tested patch now. Sorry for the re-reviews!

Regards,
Surbhi.

Thanks, merged and pushed! Unfortunately due to freeze timing, this is now going to be an SRU rather than included in the natty release pocket; I've still merged it to the 'natty' branch since that's the only available branch for pushing.

BTW, I had to refresh the patches to apply cleanly against the current plymouth source. With dpkg 3.0 (quilt) format, dpkg-source requires the patches to apply with no fuzz.