Bug 118295 - Improve gridlines option UI
Summary: Improve gridlines option UI
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: Sheet-Gridlines Options-Dialog-Calc
  Show dependency treegraph
 
Reported: 2018-06-21 08:30 UTC by Heiko Tietze
Modified: 2021-02-02 11:49 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Heiko Tietze 2018-06-21 08:30:27 UTC
Tools > options > calc > visual aids > gridlines = Show on colored cells is supposed to show the gridlines when the cell background is yellow, for example. That doesn't work at 6.0.

Furthermore, this feature is better provided as an extra option. For example like this:

*Visual Aids*
(o) Show Gridlines
( ) Hide Gridlines
    [ ] But show on colored cells

If the user has gridlines active by default but switches it off temporarily per view > gridlines, the show on colored cells might become handy as well. So we could also do it like this

*Visual Aids*
(o) Show Gridlines
( ) Hide Gridlines

[x] Always show gridlines on colored cells
Comment 1 Julien Nabet 2018-06-21 20:10:13 UTC
On pc Debian x86-64 with master sources updated today, I could reproduce this.

I noticed that this option was used with vars or const like:
- bGridOnTop
- VOPT_GRID_ONTOP

    318 IMPL_LINK( ScTpContentOptions, GridHdl, ListBox&, rLb, void )
    319 {
    320     sal_Int32   nSelPos = rLb.GetSelectedEntryPos();
    321     bool    bGrid = ( nSelPos <= 1 );
    322     bool    bGridOnTop = ( nSelPos == 1 );
    323 
    324     pColorFT->Enable(bGrid);
    325     pColorLB->Enable(bGrid);
    326     pLocalOptions->SetOption( VOPT_GRID, bGrid );
    327     pLocalOptions->SetOption( VOPT_GRID_ONTOP, bGridOnTop );
    328 }
=> 
Show : value 0, bGrid = true, bGridOnTop = false
Show on colored cells : value 1, bGrid = true, bGridOnTop = true
Hide : value 2, bGrid = false, bGridOnTop = false
See https://opengrok.libreoffice.org/xref/core/sc/source/ui/optdlg/tpview.cxx#318


Again the same mix (grid on color/grid on top):
    498             case SCLAYOUTOPT_GRID_ONCOLOR:
    499                 pValues[nProp] <<= GetOption( VOPT_GRID_ONTOP );
    500                 break;
https://opengrok.libreoffice.org/xref/core/sc/source/core/tool/viewopti.cxx#498

Finally the value is used in sc/source/ui/view/gridwin4.cxx: 
    557     bool bGridFirst = !rOpts.GetOption( VOPT_GRID_ONTOP );
...
    715     if ( bGridFirst && ( bGrid || bPage ) )
    716         aOutputData.DrawGrid(*pContentDev, bGrid, bPage);
    717 
    718     aOutputData.DrawBackground(*pContentDev);
    719 
    720     if ( !bGridFirst && ( bGrid || bPage ) )
    721         aOutputData.DrawGrid(*pContentDev, bGrid, bPage);
See https://opengrok.libreoffice.org/xref/core/sc/source/ui/view/gridwin4.cxx#715
Comment 2 Xisco Faulí 2018-06-25 09:25:12 UTC
@Heiko, @Julien,
After the wonderful comment 1 made by Julien, should we convert this into an easyHack?
Comment 3 Julien Nabet 2018-06-25 09:28:41 UTC
Xisco: after my comment 1, I'd say it's quite confusing to have 2 concepts mixed, perhaps we should ping Eike instead to have his opinion?
Comment 4 Heiko Tietze 2018-06-25 13:53:33 UTC
Don't think we need input from UX when it smells like a bug.
Comment 5 Julien Nabet 2018-06-25 14:07:22 UTC
Eike: I tried to investigate a bit but it seems there's some confusion (see comment 1). Any idea?
Comment 6 Maxim Monastirsky 2018-06-25 14:15:16 UTC
(In reply to Heiko Tietze from comment #0)
> Furthermore, this feature is better provided as an extra option. For example
> like this:
> 
> *Visual Aids*
> (o) Show Gridlines
> ( ) Hide Gridlines
>     [ ] But show on colored cells

I think that "Show on colored cells" means "Show *also* on colored cells, when grid is otherwise visible", not "Show *only* on colored cells, when the grid is otherwise invisible". The help claims that too:

Grid lines

Specifies when grid lines will be displayed. Default is to display grid lines only on cells that do not have a background color. You can choose to also display grid lines on cells with background color, or to hide them. For printing, choose Format - Page - Sheet and mark the Grid check box.
Comment 7 Heiko Tietze 2018-06-25 14:27:36 UTC
(In reply to Maxim Monastirsky from comment #6)
> I think that "Show on colored cells" means "Show *also* on colored cells,
> when grid is otherwise visible", not "Show *only* on colored cells, when the
> grid is otherwise invisible".

We get more flexibility and a clear UI with a separate option, whether it's now specific for or also on colored cells.
Comment 8 Eike Rathke 2018-06-26 12:19:18 UTC
Maxim is right, the option "Show on colored cells" means to show grid always, also on colored cells (as the help correctly states), and as such works perfectly fine so this isn't even a bug apart from a misleading wording in the listbox string. A separate option checkbox which effectively would also allow to hide grid lines except on colored cells (i.e. grid lines only on colored cells) IMHO doesn't make sense, thus the states are mutually exclusive and the current listbox is fine.
Comment 9 Julien Nabet 2018-06-26 12:24:35 UTC
Thank you Eike and Maxim!

Heiko/Xisco: so perhaps we could change the wording for example from:
"Show on colored cells"
to:
"Show even on colored cells"
?
Comment 10 Heiko Tietze 2018-06-26 14:50:01 UTC
(In reply to Julien Nabet from comment #9)
> "Show even on colored cells"

It's just a bad idea to spoil the on/off feature with another option. The better solution for the current implementation is "[x] As well for colored backgrounds", disabled when Hide is selected. 

We can close the ticket as WFM, if only a minor change to the caption is accepted; no need to bother l10n.
Comment 11 Julien Nabet 2018-06-26 15:28:23 UTC
(In reply to Heiko Tietze from comment #10)
> (In reply to Julien Nabet from comment #9)
> > "Show even on colored cells"
> 
> ...

Ok but in this case, shouldn't it be:
(o) Show Gridlines
    [ ] And show on colored cells
( ) Hide Gridlines

instead of
> (o) Show Gridlines
> ( ) Hide Gridlines
>     [ ] But show on colored cells
?
Comment 12 Heiko Tietze 2018-06-26 17:32:33 UTC
(In reply to Julien Nabet from comment #11)
> (o) Show Gridlines
>     [ ] And show on colored cells
> ( ) Hide Gridlines

Yes, that's how it is implemented. Someone native might find a better text for the checkbox.
Comment 13 Eike Rathke 2018-06-27 12:05:10 UTC
(In reply to Heiko Tietze from comment #10)
> It's just a bad idea to spoil the on/off feature with another option.
There is no on/off feature, there is a listbox with three values, of which one is missing the word "also".
Comment 14 Julien Nabet 2018-07-01 06:16:39 UTC
(In reply to Eike Rathke from comment #13)
> (In reply to Heiko Tietze from comment #10)
> > It's just a bad idea to spoil the on/off feature with another option.
> There is no on/off feature, there is a listbox with three values, of which
> one is missing the word "also".

+1 for me so it would give:
"Show also on colored cells"
Heiko/Xisco: ok for you?
Comment 15 Heiko Tietze 2018-07-02 06:27:37 UTC
(In reply to Julien Nabet from comment #14)
> (In reply to Eike Rathke from comment #13)
> > There is no on/off feature... 
> ...
> Heiko/Xisco: ok for you?

I disagree with Eike here, but feel free to ignore and go ahead.
Comment 16 QA Administrators 2019-07-03 02:42:21 UTC Comment hidden (obsolete)
Comment 17 Heiko Tietze 2021-02-01 10:57:10 UTC
The simple solution could be

[ ] Show grid lines
    [ ] Also on colored cells
    Color [ <grey> ]