Bug Hunting Session
Bug 124686 - HideWhiteSpace should be positive Show Whitespace
Summary: HideWhiteSpace should be positive Show Whitespace
Status: ASSIGNED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Shameem
URL:
Whiteboard:
Keywords: difficultyBeginner, easyHack, skillDesign, topicUI
Depends on:
Blocks: Main-Menu
  Show dependency treegraph
 
Reported: 2019-04-11 11:23 UTC by andreas_k
Modified: 2019-11-14 16:40 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 Praas 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.