Created attachment 298669 [details] [review]
mount-operation: Handle multi-line questions

Don't discard multiple lines when updating the message label.

This can also be seen when mounting an sftp host for the first time, some lines are missing.

Review of attachment 298669 [details] [review]:

Looks good to me, see screenshots in the Bug 526582, but I'm not gnome-shell maintainer, so please somebody else to look at the patch and to set patch status appropriately...

Review of attachment 298669 [details] [review]:

::: js/ui/shellMountOperation.js
@@ +50,1 @@
     let labels = message.split('\n');

Grrr, would be neat if split('\n', 2) behaved like g_strsplit() here :-(

@@ +52,3 @@
     _setLabelText(dialog.subjectLabel, labels[0]);
+    if (labels.length > 1) {
+        labels.shift();

Slightly more elegant as:

_setLabelText(dialog.subjectLabel, labels.shift())
if (labels.length > 0)
    _setlabelText(dialog.descriptionLabel, labels.join('\n'));


Not really in the scope of this bug, but looking at the GMountOperation documentation:
  "If the message contains a line break, the first line should be presented as a heading."
I wonder if the following would match expectations more closely:

  let labels = message.split('\n');

  if (labels.length > 1)  // there's a heading
      _setLabelText(dialog.subjectlabel, labels.shift());
  _setLabelText(dialog.descriptionLabel, labels.join('\n');

Created attachment 298798 [details] [review]
mount-operation: Handle multi-line questions

Don't discard multiple lines when updating the message label.

(In reply to Florian Müllner from comment #4)
> Review of attachment 298669 [details] [review] [review]:
> 
> ::: js/ui/shellMountOperation.js
> @@ +50,1 @@
>      let labels = message.split('\n');
> 
> Grrr, would be neat if split('\n', 2) behaved like g_strsplit() here :-(

Yeah, that's what I thought :-)

> 
> @@ +52,3 @@
>      _setLabelText(dialog.subjectLabel, labels[0]);
> +    if (labels.length > 1) {
> +        labels.shift();
> 
> Slightly more elegant as:
> 
> _setLabelText(dialog.subjectLabel, labels.shift())
> if (labels.length > 0)
>     _setlabelText(dialog.descriptionLabel, labels.join('\n'));

Done.

> 
> 
> Not really in the scope of this bug, but looking at the GMountOperation
> documentation:
>   "If the message contains a line break, the first line should be presented
> as a heading."
> I wonder if the following would match expectations more closely:
> 
>   let labels = message.split('\n');
> 
>   if (labels.length > 1)  // there's a heading
>       _setLabelText(dialog.subjectlabel, labels.shift());
>   _setLabelText(dialog.descriptionLabel, labels.join('\n');

I tried that, but it doesn't look good because the description text is rather small for a one line question like "Are you sure you want to continue?". Also, it doesn't line up with the icon then.

Review of attachment 298798 [details] [review]:

(In reply to Ross Lagerwall from comment #6)
>
> >   if (labels.length > 1)  // there's a heading
> >       _setLabelText(dialog.subjectlabel, labels.shift());
> >   _setLabelText(dialog.descriptionLabel, labels.join('\n');
> 
> I tried that, but it doesn't look good because the description text is
> rather small for a one line question like "Are you sure you want to
> continue?". Also, it doesn't line up with the icon then.

Yeah, I was pretty sure our dialogs wouldn't deal with a missing title ...

Thanks.