D17533: Make the button's purpose more clear

[Nathaniel Graham]

ngraham added a comment.


  @amantia, if you would like to continue with this, could you move it to 
invent.kde.org? as a merge request for plasma-desktop? Thanks!

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: pino, gladhorn, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia added a comment.


  I understand that, of course. I think the question is: is this an improvement 
or not? I think it is (not perfect, has problems, but an improvement), but if 
considered to be not, I will just abandon it, no hard feelings.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: pino, gladhorn, ngraham, plasma-devel, kvanton, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D17533: Make the button's purpose more clear

[Pino Toscano]

pino added a comment.


  I'm personally still not convinced about the usage of the bold for questions, 
as I wrote in an earlier comment.
  The rest is not up to me.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: pino, gladhorn, ngraham, plasma-devel, kvanton, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia added a comment.


  Ping? I'll push this if there are no strong objections.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: pino, gladhorn, ngraham, plasma-devel, kvanton, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia updated this revision to Diff 47714.
amantia added a comment.


  Use title case

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17533?vs=47712=47714

BRANCH
  accesbility_dialog_improvement

REVISION DETAIL
  https://phabricator.kde.org/D17533

AFFECTED FILES
  kaccess/kaccess.cpp
  kaccess/kaccess.h
  kaccess/main.cpp

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: pino, gladhorn, ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Pino Toscano]

pino added a comment.


  Regarding the use of bold: please note that using bold in some languages 
(usually eastern ones) either is not possible, or it does not make sense. So 
please do not solely rely on it to convey anything.

INLINE COMMENTS

> kaccess.cpp:671
>  dialog = new QDialog(nullptr);
> -dialog->setWindowTitle(i18n("Warning"));
> +dialog->setWindowTitle(i18n("Accessibility feature changes"));
>  dialog->setObjectName(QStringLiteral("AccessXWarning"));

Since you are changing this string: can you please use Title Capitalization as 
described in the HIG?
Also, adding a context like "@title:window" will help distinguish this as title.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: pino, gladhorn, ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia added a comment.


  I perfectly agree with Frederik. There is room for improvement, but my 
intention was not to completely change it, just to make it slightly more clear 
what this is about. The rest can be fixed in a separate commit, if anyone is 
interested to do it. :)

INLINE COMMENTS

> gladhorn wrote in kaccess.cpp:870
> I'd say updating all strings is good. Translators should have support from 
> their tools (translation memory) and we will eventually have to move forward.

I did, although disagree, as it makes the change not easily backportable. I 
also couldn't test/trigger all cases.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: gladhorn, ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia updated this revision to Diff 47712.
amantia added a comment.


  Change the individual texts

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17533?vs=47661=47712

BRANCH
  accesbility_dialog_improvement

REVISION DETAIL
  https://phabricator.kde.org/D17533

AFFECTED FILES
  kaccess/kaccess.cpp
  kaccess/kaccess.h
  kaccess/main.cpp

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: gladhorn, ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Frederik Gladhorn]

gladhorn added a comment.


  I also find the text rather confusing (thank you for working on this!).
  What is AccessX? And why should I care as a user? I need the feature...
  
  On the other hand, this patch is simple, self-contained, and in my opinion a 
clear improvement, so I'd vote for taking in this change and in parallel making 
further improvements on top of it.

INLINE COMMENTS

> amantia wrote in kaccess.cpp:870
> That looks weird., as it creates a translatable string for no reason.  
> Obviously best would be to update all strings to have the bold text in it, 
> but that creates lots of new strings for no good reason

I'd say updating all strings is good. Translators should have support from 
their tools (translation memory) and we will eventually have to move forward.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: gladhorn, ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia abandoned this revision.
amantia added inline comments.

INLINE COMMENTS

> ngraham wrote in kaccess.cpp:870
> Let's use `xi18nc()` for this:
> 
> `xi18nc("@info", "%1", question)`

That looks weird., as it creates a translatable string for no reason.  
Obviously best would be to update all strings to have the bold text in it, but 
that creates lots of new strings for no good reason

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> kaccess.cpp:870
> + QStringLiteral(" ") + i18n("These 
> AccessX settings are needed for some users with motion impairments and can be 
> configured in the KDE System Settings. You can also turn them on and off with 
> standardized keyboard gestures.\n\nIf you do not need them, you can select 
> \"Deactivate all AccessX features and gestures\"."));
> -
> +questionLabel->setText("" + question + "");
>  KWindowSystem::setState(dialog->winId(), NET::KeepAbove);

Let's use `xi18nc()` for this:

`xi18nc("@info", "%1", question)`

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia added a comment.


  Yes, most questions can be answered with Yes/No, and still I'd say it is 
better to use Yes/No, instead of OK/Cancel. OK/Cancel is rather useless, 
although widely used, I know...
  I made the question text bold now, and unfortunately as I said I don't want 
to spend more on this. The whole dialog is problematic as it poses both a 
question (activate/do not activate) and a side question (turn off gestures or 
not). 
  This is confusing by default, and for the users it was unclear what the 
dialog buttons do, as the original question was at the beginning and between 
the question and the action buttons there was the large explanation with the 
secondary action.
  At least (IMO) now it is clear that the Yes/No buttons refer to the 
activate/deactivate question.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia updated this revision to Diff 47661.
amantia added a comment.


  Make the question text bold

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17533?vs=47590=47661

BRANCH
  accesbility_dialog_improvement

REVISION DETAIL
  https://phabricator.kde.org/D17533

AFFECTED FILES
  kaccess/kaccess.cpp
  kaccess/kaccess.h
  kaccess/main.cpp

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Nathaniel Graham]

ngraham added a comment.


  All dialogs ask a question that can be answered with a yes or a no. The 
problem is that most users don't read dialog box text--especially very long 
text, like the kind presented here. They do generally read the dialog box 
button labels though, which is why it's so important that the labels indicate 
what the buttons do. I can understand if you don't want to do that in this 
patch though.
  
  IMHO we should work towards allowing the button labels to be customized 
alongside the dialog box text. As is, this patch doesn't really improve much 
because the text is already so long that nobody will read it, no matter which 
order the strings are presented in. If there's no way to simplify the text, 
perhaps we could should use bold text for the important part? This is what we 
do for Dolphin's "permanently delete this file" dialog:
  F6478899: Screenshot_20181215_140503.png 


REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia added a comment.


  I'm not a native speaker, but I don't see how OK/Cancel is better. The dialog 
shows a question that can be answered with Yes or No. 
  See also the HIG https://hig.kde.org/components/assistance/message.html
  
  > - Apply confirmation button labels when further interaction is required:
  > 
  >   Use buttons which match the type of statement or question made in the 
warning or error message. **For example, do no ask a Yes/No question but then 
provide OK/Cancel buttons.**
  > - Apply confirmation button labels when the user must choose between two 
actions to continue:
  > 
  >   Use descriptive button labels instead of standard Yes/No or OK/Cancel 
buttons. For example, if the user must choose to continue or stop an action, 
provide the buttons “Continue” and “Cancel”.
  
  I cannot easily do the second (and don't want to with this patch).

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Nathaniel Graham]

ngraham added a comment.


  In D17533#376922 , @amantia wrote:
  
  > You can't change to Yes/No (that was my first try), as the question might 
be one of the following:
  >
  > - "Activate foo ?"
  > - "Deactivate foo?"
  > - "Activate foo and deactivate bar?"
  
  
  In that case, OK/Cancel would at least be better than Yes/No.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia updated this revision to Diff 47590.
amantia added a comment.


  Improve dialog title as well

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17533?vs=47441=47590

BRANCH
  accesbility_dialog_improvement

REVISION DETAIL
  https://phabricator.kde.org/D17533

AFFECTED FILES
  kaccess/kaccess.cpp
  kaccess/kaccess.h
  kaccess/main.cpp

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  While we're here, let's change yes to Accept and No to Cancel. Also, the 
dialog's title should probably be something more descriptive than "Warning".

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D17533: Make the button's purpose more clear

[Andras Mantia]

amantia added a comment.


  You can't change to Yes/No (that was my first try), as the question might be 
one of the following:
  
  - "Activate foo ?"
  - "Deactivate foo?"
  - "Activate foo and deactivate bar?"

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D17533

To: amantia, #plasma_accessibility, davidedmundson, aacid, ngraham
Cc: ngraham, plasma-devel, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart