Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make row color alternating in the Peers tab optional #307

Closed
wants to merge 3 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 1, 2021

This PR:

Screenshot:

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

@hebasto
Copy link
Member Author

hebasto commented May 1, 2021

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, thank you for adding this flexibility.

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved
@hebasto
Copy link
Member Author

hebasto commented May 1, 2021

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

@hebasto
Copy link
Member Author

hebasto commented May 1, 2021

Updated fe7d615 -> b7de85d (pr307.02 -> pr307.03, diff):

@hebasto hebasto changed the title Make row color alternating in the Peers Tab switchable Make row color alternating in the Peers tab switchable May 1, 2021
@hebasto hebasto changed the title Make row color alternating in the Peers tab switchable Make row color alternating in the Peers tab optional May 1, 2021
@rebroad
Copy link
Contributor

rebroad commented May 1, 2021

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-

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

@hebasto
Copy link
Member Author

hebasto commented May 2, 2021

  1. No stripes by default, but a config option somewhere more hidden than this proposed option (maybe in a config file instead?)
  1. What is the rationale of hiding a visual appearance option from users?

  2. The bitcoin.conf is not dedicated for such options.

@promag
Copy link
Contributor

promag commented May 2, 2021

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
Copy link
Member

jonatack commented May 2, 2021

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
Copy link
Member

jarolrod commented May 3, 2021

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
Copy link
Contributor

Concept ACK

@hebasto hebasto added the UI All about "look and feel" label May 9, 2021
@hebasto
Copy link
Member Author

hebasto commented May 10, 2021

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

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 081842d

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
Screen Shot 2021-05-12 at 11 12 33 PM Screen Shot 2021-05-12 at 11 13 15 PM

Restart GUI:

Start: Setting Persists ✅ Disable Option
Screen Shot 2021-05-12 at 11 20 19 PM Screen Shot 2021-05-12 at 11 20 37 PM

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2021

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

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2021

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

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK 3231689 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)

@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2021

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

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Screenshots:

Start Enable
307-start 307-enable
Restart Disable
307-restart 307-disable

@@ -6,8 +6,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>560</width>
<height>440</height>
<width>646</width>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
size-increase size-normal

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PruneSize, // int
DatabaseCache, // int
SpendZeroConfChange, // bool
Listen, // bool
OptionIDRowCount,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit in 217d42b

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hebasto
Copy link
Member Author

hebasto commented Jun 11, 2021

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

@ghost
Copy link

ghost commented Jun 14, 2021

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
Copy link
Member

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

@hebasto hebasto closed this Jun 14, 2021
@Rspigler
Copy link
Contributor

Sad to see this closed. Concept ACK from me.

@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants