[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 108480: Always confirm "Leave..." from desktop context menu
From: "Marco Martin" <notmart () gmail ! com>
Date: 2013-01-22 9:31:40
Message-ID: 20130122093140.18956.40070 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jan. 20, 2013, 2:34 a.m., Maarten De Meyer wrote:
> > This change is wrong imho. If 'Confirm logout' is disabled it should not ask for \
> > confirmation. Otherwise what's the point of that option? Showing the dialog \
> > depending on the button clicked breaks UI expectations a lot more than the \
> > presence of ellipses. Perhaps remove the ellipses if the confirmation is \
> > disabled? (And show them in Kickoff/Tool box when enabled) If people keep losing \
> > work due to hitting 'leave' by accident, there is a solution for that: Confirm \
> > logout.
> > Note: just my personal opinion and I'm not a plasma developer.
>
> Marco Martin wrote:
> i tend to agree with Marteen.
> I think is not entirely nice that option is here at all since can have potentially \
> dangerous results, but since is there it has to have a coherent behavior \
> everywhere.
> maybe what could be done is to remove the "..." in case confirmation is disabled
>
> Aaron J. Seigo wrote:
> +1 on removing the '...' when this setting is on. will drop this review as a \
> result.
> thanks for the submission, though, Jacob!
>
> Jacob Welsh wrote:
> I can see the logic to this, however it raises some more issues. 1) The \
> translations for "Leave..." would have to be changed, and the "..." appended \
> programatically depending on the setting. That might not even make sense for all \
> languages... what about right-to-left? If not, both versions would have to be \
> translated. And same for the kickoff items. If this is the best solution though, I \
> guess we'll just have to deal with it. 2) The "Leave" button is not quite the same \
> as the kickoff options, because they explicitly specify a particular form of \
> leaving. "Leave" is not clear whether it means shutdown, logout, sleep or what. \
> This is why it makes sense to me to always prompt: how would you like to leave? \
> Whereas with kickoff, the user has already provided that info, so the prompt is \
> just a timeout to prevent accidents (which I'd prefer to disable in my own setup). \
> Other solutions to #2 might be adding explicit shutdown/logout options to the \
> desktop context menu (cluttering
it), or having the text change based on the default leave option (complicating the \
code quite a bit).
for the translation, it would of course have to be two different strings, \
i18n("Leave...") and i18n("Leave"), the onle way it can be sure it works on any \
language
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108480/#review25817
-----------------------------------------------------------
On Jan. 19, 2013, 7:37 a.m., Jacob Welsh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108480/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2013, 7:37 a.m.)
>
>
> Review request for Plasma.
>
>
> Description
> -------
>
> Description from bug:
>
> There's a logout option in the desktop context menu named "Leave...". The presence \
> of ellipses indicates to the user that there will be a confirmation dialog. \
> However, if you've disabled "Confirm logout" in System Settings / Startup and \
> Shutdown / Session Management, no such confirmation will be given. This breaks UI \
> expectations and could lead to loss of unsaved work. (I still prefer to disable \
> confirmation for the case of the kickoff menu items, which don't have ellipses and \
> are harder to hit "by accident".)
>
> This addresses bug 313480.
> http://bugs.kde.org/show_bug.cgi?id=313480
>
>
> Diffs
> -----
>
> plasma/generic/containmentactions/contextmenu/menu.cpp bfd60a1
>
> Diff: http://git.reviewboard.kde.org/r/108480/diff/
>
>
> Testing
> -------
>
> None. Sorry, I gave up on getting all the KDE components built from git for the \
> time being; was getting one error after another. If there's a quick way to test \
> just the plasma workspace I'd be happy to try.
>
> Thanks,
>
> Jacob Welsh
>
>
[Attachment #5 (text/html)]
<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/108480/">http://git.reviewboard.kde.org/r/108480/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On January 20th, 2013, 2:34 a.m. UTC, <b>Maarten \
De Meyer</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;">This change is wrong imho. If 'Confirm logout' is disabled it \
should not ask for confirmation. Otherwise what's the point of that option? \
Showing the dialog depending on the button clicked breaks UI expectations a lot more \
than the presence of ellipses. Perhaps remove the ellipses if the confirmation is \
disabled? (And show them in Kickoff/Tool box when enabled) If people keep losing work \
due to hitting 'leave' by accident, there is a solution for that: Confirm \
logout.
Note: just my personal opinion and I'm not a plasma developer.</pre>
</blockquote>
<p>On January 21st, 2013, 9:34 a.m. UTC, <b>Marco Martin</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 tend to agree with \
Marteen. I think is not entirely nice that option is here at all since can have \
potentially dangerous results, but since is there it has to have a coherent behavior \
everywhere.
maybe what could be done is to remove the "..." in case confirmation is \
disabled</pre> </blockquote>
<p>On January 21st, 2013, 12:22 p.m. UTC, <b>Aaron J. Seigo</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;">+1 on removing the \
'...' when this setting is on. will drop this review as a result.
thanks for the submission, though, Jacob!</pre>
</blockquote>
<p>On January 21st, 2013, 11:32 p.m. UTC, <b>Jacob Welsh</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 can see the logic to \
this, however it raises some more issues. 1) The translations for \
"Leave..." would have to be changed, and the "..." appended \
programatically depending on the setting. That might not even make sense for all \
languages... what about right-to-left? If not, both versions would have to be \
translated. And same for the kickoff items. If this is the best solution though, I \
guess we'll just have to deal with it. 2) The "Leave" button is not \
quite the same as the kickoff options, because they explicitly specify a particular \
form of leaving. "Leave" is not clear whether it means shutdown, logout, \
sleep or what. This is why it makes sense to me to always prompt: how would you like \
to leave? Whereas with kickoff, the user has already provided that info, so the \
prompt is just a timeout t o prevent accidents (which I'd prefer to disable in \
my own setup). Other solutions to #2 might be adding explicit shutdown/logout options \
to the desktop context menu (cluttering it), or having the text change based on the \
default leave option (complicating the code quite a bit).</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">for the translation, it \
would of course have to be two different strings, i18n("Leave...") and \
i18n("Leave"), the onle way it can be sure it works on any language</pre> \
<br />
<p>- Marco</p>
<br />
<p>On January 19th, 2013, 7:37 a.m. UTC, Jacob Welsh wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Plasma.</div>
<div>By Jacob Welsh.</div>
<p style="color: grey;"><i>Updated Jan. 19, 2013, 7:37 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;">Description from bug:
There's a logout option in the desktop context menu named "Leave...". \
The presence of ellipses indicates to the user that there will be a confirmation \
dialog. However, if you've disabled "Confirm logout" in System Settings \
/ Startup and Shutdown / Session Management, no such confirmation will be given. This \
breaks UI expectations and could lead to loss of unsaved work. (I still prefer to \
disable confirmation for the case of the kickoff menu items, which don't have \
ellipses and are harder to hit "by accident".)</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;">None. Sorry, I gave up on getting all the KDE components built from git \
for the time being; was getting one error after another. If there's a quick way \
to test just the plasma workspace I'd be happy to try.</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=313480">313480</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>plasma/generic/containmentactions/contextmenu/menu.cpp <span style="color: \
grey">(bfd60a1)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108480/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic