Bug 124686 - HideWhiteSpace should be positive Show Whitespace
Summary: HideWhiteSpace should be positive Show Whitespace
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Shameem
URL:
Whiteboard: target:7.0.0
Keywords: difficultyBeginner, easyHack, skillDesign, topicUI
Depends on:
Blocks: Main-Menu
  Show dependency treegraph
 
Reported: 2019-04-11 11:23 UTC by andreas_k
Modified: 2022-11-25 09:25 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments
view menubar (39.68 KB, image/png)
2019-04-11 11:23 UTC, andreas_k
Details
Minimal fix without changing all "hidewhitespace" string (1.53 KB, patch)
2019-11-11 19:20 UTC, Shameem
Details

Note You need to log in before you can comment on or make changes to this bug.
Description andreas_k 2019-04-11 11:23:41 UTC
Created attachment 150697 [details]
view menubar

writer -> menubar -> View -> Hide Whitespace

all other toggles boxes are activated to show x only hide whitespace is active when something is hidden.

also the guideline say that checkboxes should be activated when something is shown not hidden.
Comment 1 Dieter 2019-04-11 11:41:49 UTC
Andreas, I hope you agree that "Show Whitespace" should be activated by default.
Comment 2 Heiko Tietze 2019-04-11 11:48:08 UTC
HIG is "Turning on an item in the menu should always enable the option. Negative options create a double negative which can be confusing. For example, use 'Show hidden files' instead of 'Hide shown files'."

Of course, the default needs to get changed too.

Removing UX, and it's an easyhack.
Comment 3 andreas_k 2019-04-11 11:58:06 UTC
(In reply to Dieter Praas from comment #1)
> Andreas, I hope you agree that "Show Whitespace" should be activated by
> default.

for sure as most other items are activated by default.
Comment 4 Andreas Heinisch 2019-05-21 16:47:54 UTC
At the moment I am not very familiar with the LO codebase. Just a short question: does this change only affect the sw folder or are there any other positions to consider?
Comment 5 Heiko Tietze 2019-05-22 15:08:54 UTC
Hm, maybe it's a bit more work...

https://opengrok.libreoffice.org/search?project=core&full=HideWhitespace&defs=&refs=&path=&hist=&type=

(not sure all are relevant as some files are for pdf export)
Comment 6 Andreas Heinisch 2019-05-22 18:04:01 UTC
Thank you very much! I have already seen all these files and I am afraid that I break something in another part of LO. At the moment my code works, but I have to do some other work, until I can submit a patch. Busy times ...
Comment 7 Andreas Heinisch 2019-06-03 07:50:29 UTC
I have some small questions about this change:
- Should there be changed the whole backend including function names, function calls, variables etc. or should there just changed the ui behaviour?
- How I handle the translation of the new label? Unfortenatley, I did not find any good documentation about adding translation to the code base
Comment 8 Heiko Tietze 2019-06-03 08:07:32 UTC
(In reply to Andreas Heinisch from comment #7)
> I have some small questions about this change:
> - Should there be changed the whole backend including function names,
> function calls, variables etc. or should there just changed the ui behaviour?
> - How I handle the translation of the new label? Unfortenatley, I did not
> find any good documentation about adding translation to the code base

Code is up to the developer, if you are unsure the IRC channel or mailing list is the right place to ask. I would keep it simple.
If you change strings in the hrc files (NC_) it goes automatically to the localization. No need to trigger anything there.
Comment 9 Xisco Faulí 2019-09-04 11:24:10 UTC
Dear Andreas Heinisch,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assigned it back to yourself if you're still working on this.
Comment 10 Shameem 2019-10-12 13:15:20 UTC
(In reply to Heiko Tietze from comment #8)
> If you change strings in the hrc files (NC_) it goes automatically to the
> localization. No need to trigger anything there.

What do you mean by hrc files?
Comment 11 Heiko Tietze 2019-10-13 11:48:05 UTC
Don't hesitate to ask in case of questions, Muhammed.
Comment 12 Heiko Tietze 2019-10-14 08:09:29 UTC
(In reply to Shameem from comment #10)
> What do you mean by hrc files?

We store l10n variables in *.hrc files [see 1] however not in this case. My comment was about not to just rename the UNO command from Hide* to Show* but also to invert the internal logic, which makes it a bit more tricky.

Some code pointers:

.uno:HideWhitespace
https://opengrok.libreoffice.org/xref/core/officecfg/registry/data/org/openoffice/Office/UI/WriterCommands.xcu?r=1496a183#2708
https://opengrok.libreoffice.org/xref/core/sw/sdi/swriter.sdi?r=1496a183#5387

FN_VIEW_HIDE_WHITESPACE:
https://opengrok.libreoffice.org/xref/core/sw/source/uibase/uiview/view0.cxx?r=6c6c1eea#293
https://opengrok.libreoffice.org/xref/core/sw/source/uibase/uiview/view0.cxx?r=6c6c1eea#457

[1] https://opengrok.libreoffice.org/search?project=core&full=&defs=&refs=&path=hrc&hist=&type=&si=path
Comment 13 Shameem 2019-11-07 14:09:21 UTC
I didn't get enough time to look into this last two weeks.

I am back at this now. Just completed the cosmetic changes. Still looking at code to implement the logic.
Comment 14 Shameem 2019-11-11 19:20:05 UTC
Created attachment 155714 [details]
Minimal fix without changing all "hidewhitespace" string

This patch will achieve the goal with minimal changes. Changing all occurrence of "hidewhitespace" is a pain as mentioned in comment 5.
Comment 15 Shameem 2019-11-14 16:40:15 UTC
I am in the process of renaming all the occurrences of "FN_VIEW_HIDE_WHITESPACE" and "HideWhitespace". I am done with renaming FN_VIEW_HIDE_WHITESPACE to FN_VIEW_SHOW_WHITESPACE.
Comment 16 Xisco Faulí 2020-02-17 10:46:55 UTC
Dear Shameem,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assign it back to yourself if you're still working on this.
Comment 17 Shameem 2020-03-11 12:03:47 UTC
Under review https://gerrit.libreoffice.org/c/core/+/90324
Comment 18 Commit Notification 2020-03-30 07:37:21 UTC
shameempk committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/96def0ee391a99ddf042df820db1292b83d0c4fe

tdf#124686 HideWhiteSpace should be positive Show Whitespace

It will be available in 7.0.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 19 Dieter 2020-05-02 15:39:56 UTC
Verified with

Version: 7.0.0.0.alpha0+ (x64)
Build ID: b8fb7ecd9cdbe1898c41eaecd9894df8e8f01e25
CPU threads: 4; OS: Windows 10.0 Build 18363; UI render: Skia/Raster; VCL: win; 
Locale: de-DE (de_DE); UI-Language: en-GB
Calc: threaded

Shameem, thanks for fixing it!

One additional remark: All other entries in the menu doesn't start with "show". So to be in line with other entries, it should simple called "Whitespace". Sorry, but I wasn't aware of it before. Shameem, is it possible without much effort to change just the name in the menu?
Comment 20 Heiko Tietze 2020-05-02 16:01:23 UTC
(In reply to Dieter from comment #19)
> So to be in line with other entries, it should simple called "Whitespace". 

Rather the opposite and always have the action/verb at the beginning.
Comment 21 Dieter 2020-05-03 04:32:04 UTC
(In reply to Heiko Tietze from comment #20)
> Rather the opposite and always have the action/verb at the beginning.

Sorry Heiko, but I don't understand your comment. Only the entry "Show Whitespace" has a verb within the View menu. So why "Show Whitepace" and not "Show Field Names", "Show Sidebar", "Show Track Changes" (see bug 132168 coment 9), "Show Styles"? So I would simply skip the veb "show" (no change in functionality). But that's not a big deal. If you disagree, leave it as it is.
Comment 22 Heiko Tietze 2020-05-03 06:36:34 UTC
(In reply to Dieter from comment #21)
> I would simply skip the verb "show"

In the narrow scope it's clear what you mean with Whitespace. But let's have bug 91874 implemented, for example, and search for a function. There might be Insert > Add Whitespace and View > Show Whitespace as well as Format > Convert Whitespace. The second argument is that only the checkboxes next to the menu entries show the toggle functionality. Show implies it textually in addition. Having the action listed next to the object is just good usability in my opinion. I admit that a text wall with Show* looks awkward.
Comment 23 Dieter 2020-05-03 10:26:52 UTC
Thanks for explanation
=> VERIFIED FIXED
Comment 24 andreas_k 2020-05-03 10:39:20 UTC
Thanks for fix the bug
Comment 25 Dieter 2020-05-03 11:09:11 UTC
Request for change in LO Help: Bug 132630
Comment 26 Rizal Muttaqin 2020-05-31 01:32:55 UTC
Now this new Show Whitespace missed the icons