Make row color alternating in the Peers tab optional

hebasto commented at 2:20 pm on May 1, 2021: member

This PR:

makes an “alternating row colors” feature in the Peers tab switchable to satisfy all users with different tastes.
is a follow up of #298
is an alternative to #306

Screenshot:

Screenshot from 2021-05-01 18-56-44

hebasto commented at 2:22 pm on May 1, 2021: member

cc: @rebroad @jonatack @RandyMcMillan @Bosch-0 @jarolrod

in src/qt/forms/optionsdialog.ui:811 in 8551215a07 outdated

804@@ -805,6 +805,16 @@
805          </layout>
806         </widget>
807        </item>
808+       <item>
809+        <widget class="QCheckBox" name="peersTabAlternatingRowColors">
810+         <property name="toolTip">
811+          <string>Turn on/off row color alternating for both &quot;Peers&quot; and &quot;Banned peers&quot; tables in the Peer Tab.</string>

jonatack commented at 3:44 pm on May 1, 2021:

0          <string>Alternate the row colors for the &quot;Peers&quot; and &quot;Banned peers&quot; tables in the Peers tab.</string>

hebasto commented at 3:53 pm on May 1, 2021:

Thanks! Updated.

jonatack commented at 3:45 pm on May 1, 2021: contributor

Concept ACK, thank you for adding this flexibility.

hebasto force-pushed on May 1, 2021

hebasto commented at 3:50 pm on May 1, 2021: member

Updated 8551215a071244ed28fa4b2f426a1dab82743bf8 -> fe7d615cd8d746d6b36e3243f10ae5b954d1e544 (pr307.01 -> pr307.02, diff):

addressed @jonatack’s comment

in src/qt/forms/optionsdialog.ui:814 in fe7d615cd8 outdated

809+        <widget class="QCheckBox" name="peersTabAlternatingRowColors">
810+         <property name="toolTip">
811+          <string>Alternate the row colors for the &quot;Peers&quot; and &quot;Banned peers&quot; tables in the Peers tab.</string>
812+         </property>
813+         <property name="text">
814+          <string>Alternate row colors in the Peers Tab</string>

jonatack commented at 3:50 pm on May 1, 2021:

s/Tab/tab/

jonatack commented at 3:51 pm on May 1, 2021:

(or change Overview s/tab/Tab/ but lowercase seems fine)

hebasto commented at 3:52 pm on May 1, 2021:

Thanks. Updated.

hebasto force-pushed on May 1, 2021

hebasto commented at 3:52 pm on May 1, 2021: member

Updated fe7d615cd8d746d6b36e3243f10ae5b954d1e544 -> b7de85d51c503576514d6d9cef0fbc2eb00e8aea (pr307.02 -> pr307.03, diff):

addressed @jonatack’s comment

hebasto renamed this:
~~Make row color alternating in the Peers Tab switchable~~
Make row color alternating in the Peers tab switchable
on May 1, 2021

hebasto renamed this:
~~Make row color alternating in the Peers tab switchable~~
Make row color alternating in the Peers tab optional
on May 1, 2021

rebroad commented at 10:35 pm on May 1, 2021: contributor

I’m sorry for being picky..! Or a diva… or whatever it is…

Just to clarify, I think my preferences are, in order of preference-

No stripes by default, but a config option somewhere more hidden than this proposed option (maybe in a config file instead?)
Stripes by default, but a config option somewhere more hidden than this proposed option.
No stripes, no config option anywhere
Stripes, no config option anywhere
This proposed pull request.

hebasto commented at 7:23 am on May 2, 2021: member

No stripes by default, but a config option somewhere more hidden than this proposed option (maybe in a config file instead?)

What is the rationale of hiding a visual appearance option from users?
The bitcoin.conf is not dedicated for such options.

promag commented at 9:43 pm on May 2, 2021: contributor

So far we had alternate rows and nobody complained AFAIK. If we add this option I think it should affect all tables, not only peers table.

Concept ACK figuring out the scope of appearance settings. Until then NACK, this particular option seems to have no grounds and might lead to similar pull requests.

jonatack commented at 9:49 pm on May 2, 2021: contributor

I think this is a fantastic first step back to a cleaner UI and it would be even better as a global option in a follow-up.

jarolrod commented at 6:25 am on May 3, 2021: member

I agree with this

If we add this option I think it should affect all tables, not only peers table.

Also, this sets a good precedent for continuing to make the GUI user configurable.

RandyMcMillan commented at 6:16 am on May 6, 2021: contributor

Concept ACK

hebasto added the label UI on May 9, 2021

hebasto force-pushed on May 10, 2021

hebasto commented at 9:18 pm on May 10, 2021: member

Rebased b7de85d51c503576514d6d9cef0fbc2eb00e8aea -> 081842d6a735a85e5611ab36703b63f8632f7f4b (pr307.03 -> pr307.04) due to the conflict with #194.

jarolrod commented at 3:22 am on May 13, 2021: member

ACK 081842d6a735a85e5611ab36703b63f8632f7f4b

Thanks for adding this option and tidying up the spacing in optionsdialog.ui

If we add this option I think it should affect all tables, not only peers table.

This can be addressed in a follow-up PR.

Start up GUI:

Start	Enable Option

Restart GUI:

Start: Setting Persists ✅	Disable Option

DrahtBot added the label Needs rebase on Jun 1, 2021

hebasto force-pushed on Jun 5, 2021

hebasto commented at 10:49 am on June 5, 2021: member

Rebased 081842d6a735a85e5611ab36703b63f8632f7f4b -> 2bc53b62494b24388923aa2697443949b8bbbba7 (pr307.04 -> pr307.05) due to the conflict with #123.

DrahtBot removed the label Needs rebase on Jun 5, 2021

hebasto force-pushed on Jun 5, 2021

hebasto commented at 12:35 pm on June 5, 2021: member

Rebased 2bc53b62494b24388923aa2697443949b8bbbba7 -> 3231689fa9b379e2c214a69b6694340f22c2a7ae (pr307.05 -> pr307.06) due to the conflict with #325.

jonatack commented at 1:56 pm on June 5, 2021: contributor

Tested ACK 3231689fa9b379e2c214a69b6694340f22c2a7ae modulo that it would be nice for the change to occur immediately when checking/unchecking the option and I would squash the last 2 commits.

Wishlist:

The options dialog opens to the last tab used
Further options for toggling alternate row coloring in other places (or globally)

DrahtBot added the label Needs rebase on Jun 5, 2021

hebasto force-pushed on Jun 5, 2021

hebasto commented at 8:35 pm on June 5, 2021: member

Rebased 3231689fa9b379e2c214a69b6694340f22c2a7ae -> 84be4844064d8e1bc973a5f5c662d2069589e1c2 (pr307.06 -> pr307.07) due to the conflict with #256.

DrahtBot removed the label Needs rebase on Jun 5, 2021

jarolrod commented at 11:08 pm on June 5, 2021: member

ACK 84be484

Tested on Arch Linux Qt 5.15.2, macOS 11.3 Qt 5.15.2, and cross-compile to Windows 10.

Notes on commits:

ada34cfee3f8696d85f75353a0979effe194746b
- Confirmed that the new indentation is correct
217d42b7465bf210511ef6cdff0475eb0a9e078c
- Code Review ACK, couple of nits (1, 2)
84be4844064d8e1bc973a5f5c662d2069589e1c2
- Cherry-picking the first two commits then running clang-format-diff.py gives me the same diff

Screenshots:

Start	Enable

Restart	Disable

in src/qt/forms/optionsdialog.ui:9 in 84be484406 outdated

 5@@ -6,8 +6,8 @@
 6    <rect>
 7     <x>0</x>
 8     <y>0</y>
 9-    <width>560</width>
10-    <height>440</height>
11+    <width>646</width>

jarolrod commented at 11:11 pm on June 5, 2021:

Nit

there’s not really a reason to change the size here. I guess this is a good place to answer a question:

“When adding new settings, should we maintain a certain level of padding between the last setting option and its enclosing box?”

Use PR Size	Use Master Size

hebasto commented at 10:13 am on June 6, 2021:

there’s not really a reason to change the size here. I guess this is a good place to answer a question:

The reason is that successive openings and closings of the optionsdialog.ui in Qt Designer won’t suggest size changes.

in src/qt/optionsmodel.h:72 in 84be484406 outdated

89+        Prune,                        // bool
90+        PruneSize,                    // int
91+        DatabaseCache,                // int
92+        SpendZeroConfChange,          // bool
93+        Listen,                       // bool
94         OptionIDRowCount,

jarolrod commented at 11:13 pm on June 5, 2021:

Nit in 217d42b7465bf210511ef6cdff0475eb0a9e078c

While here, you could document the type here is int.

Additionally, if you have to retouch, you could rename the commit from: gui: Add "Alternating Row Color" settings for the Peers Tab

to:

qt: Add "Alternating Row Color" settings for the Peers Tab

hebasto commented at 6:46 am on June 11, 2021:

While here, you could document the type here is int.

I don’t think it is required to document the type of a counter, as it is not an option id.

hebasto commented at 6:50 am on June 11, 2021:

Additionally, if you have to retouch, you could rename the commit from: gui: Add "Alternating Row Color" settings for the Peers Tab

to:

qt: Add "Alternating Row Color" settings for the Peers Tab

Done in the recent push.

DrahtBot added the label Needs rebase on Jun 9, 2021

qt, refactor: Fix indentation in optionsdialog.ui b884c21a3d

qt: Add "Alternating Row Color" settings for the Peers Tab b124c2fe60

qt, refactor: Apply clang-format to the OptionsModel::OptionID enum fdf80937d1

hebasto force-pushed on Jun 11, 2021

hebasto commented at 6:49 am on June 11, 2021: member

Rebased 84be4844064d8e1bc973a5f5c662d2069589e1c2 -> fdf80937d1cc2eb7faddbc6d5ffcdb66659fb758 (pr307.07 -> pr307.08) due to the conflict with #4.

DrahtBot removed the label Needs rebase on Jun 11, 2021

ghost commented at 10:07 am on June 14, 2021: none

So far we had alternate rows and nobody complained AFAIK. If we add this option I think it should affect all tables, not only peers table.

Until then NACK, this particular option seems to have no grounds and might lead to similar pull requests.

Also NACK. First, alternate rows are added, then an option is added to disable it. Do not start option bloating because of groundless “tastes” of some users. Keep it simple (and unflexible).

jonatack commented at 10:21 am on June 14, 2021: contributor

If we don’t want to have options for accessibility then ISTM the striping should be removed globally.

hebasto closed this on Jun 14, 2021

Rspigler commented at 6:20 pm on June 15, 2021: contributor

Sad to see this closed. Concept ACK from me.

bitcoin-core locked this on Aug 16, 2022

Make row color alternating in the Peers tab optional #307