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

Enable wordWrap for Services #293

Merged
merged 1 commit into from
May 10, 2021
Merged

Enable wordWrap for Services #293

merged 1 commit into from
May 10, 2021

Conversation

RandyMcMillan
Copy link
Contributor

@RandyMcMillan RandyMcMillan commented Apr 24, 2021

Enable wordWrap for peers-tab detailView Services

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Apr 25, 2021

Blanks instead:

Screen Shot 2021-04-24 at 7 55 47 PM (2)

Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

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

tACK 84612fa, tested on Debian Sid amd64.
PR seems reasonable, that line is really usually long.
I don't care about changing to dots and spaces, IMO good enough as it is, but wouldn't NACK other variants.

@hebasto hebasto added the Design label Apr 25, 2021
@jonatack
Copy link
Contributor

Concept ACK, this seems very useful. I think either the current " & " or a ", " separator would be good. Will test.

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK 84612fa. I agree that current "&" separator is ok.

Copy link
Contributor

@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.

ACK 84612fa

Here or in a follow-up, it may be a good idea to use either a non-breaking space before the ampersand or a comma-space to avoid the separator splitting to a line by itself. Otherwise this is a great screen space-saver. Thanks!

Screenshot from 2021-04-25 15-59-44

@RandyMcMillan
Copy link
Contributor Author

I experimented with different ways to try to control where it breaks. I will take another look.

@jonatack
Copy link
Contributor

@RandyMcMillan you have two tested ACKs here and it's a nice compact self-contained diff that adds value, so if you find something for that I think you could propose it separately.

@hebasto
Copy link
Member

hebasto commented Apr 25, 2021

Concept ACK.

@RandyMcMillan

Thanks for working on this! It has been in my TODO list for a long time :)

What if to add another tiny change:

--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -708,7 +708,7 @@ QString formatServicesStr(quint64 mask)
     }
 
     if (strList.size())
-        return strList.join(" & ");
+        return strList.join("\n");
     else
         return QObject::tr("None");
 }

?

We could get:
Screenshot from 2021-04-25 19-04-21

Btw, @jonatack's alternative suggestions

Here or in a follow-up, it may be a good idea to use either a non-breaking space before the ampersand or a comma-space to avoid the separator splitting to a line by itself.

also could be implemented in the same line :)

@hebasto
Copy link
Member

hebasto commented Apr 25, 2021

@RandyMcMillan For nice commit history do you mind changing the prefix in the commit message s/peers-tab:/qt:/ ?

See: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request

@RandyMcMillan RandyMcMillan changed the title peers-tab: enable wordWrap for Services qt: enable wordWrap for Services Apr 25, 2021
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK bd04efe, tested on Linux Mint 20.1 (Qt 5.12.8):

DeepinScreenshot_select-area_20210425193959

@hebasto hebasto changed the title qt: enable wordWrap for Services Enable wordWrap for Services Apr 25, 2021
@jonatack
Copy link
Contributor

Now it will always use multiple lines even when there is enough horizontal space to only use one?

@hebasto
Copy link
Member

hebasto commented Apr 25, 2021

Now it will always use multiple lines even when there is enough horizontal space to only use one?

Right. Is it bad?

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

re-ACK bd04efe. I like this approach with always splitting by newline better than any separator character.

image

@jonatack
Copy link
Contributor

Right. Is it bad?

It just trades one dimension for the other. The flexibility of optimizing the available space was what appealed to me.

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 bd04efe

Tested on macOS 11.2.3 Qt 5.15.2

Screen Shot 2021-04-25 at 1 03 11 PM

@jonatack

Now it will always use multiple lines even when there is enough horizontal space to only use one?

I think this is ok

@hebasto
Copy link
Member

hebasto commented Apr 25, 2021

The flexibility of optimizing the available space was what appealed to me.

You're right, and I agree with you, ceteris paribus. But in this case no other row requires wider space. Therefore, making the column wider is not an optimal way to use the available area.

@jonatack
Copy link
Contributor

jonatack commented Apr 25, 2021

If the user prefers a wider space to a higher one, the flexible version allows the user to do it with no downside.

Edit: I'd suggest the comma-space separator as it requires fewer characters and is a well-understood separator.

@jonatack
Copy link
Contributor

jonatack commented Apr 25, 2021

Great suggestion with using "\n" - it eliminates the issue. :)

It only adds vertical size to reduce the horizontal size, I don't see why it's better than the flexibility of the original version to allow the user to optimize the space. :shrugging:

@hebasto
Copy link
Member

hebasto commented Apr 25, 2021

If the user prefers a wider space to a higher one, the flexible version allows the user to do it with no downside.

Edit: I'd suggest the comma-space separator as it requires fewer characters and is a well-understood separator.

The downside of such approach is that some services, that are not placed in the beginning of the line, could just miss user's attention. Of course, it's a minor one.

@RandyMcMillan
Copy link
Contributor Author

I lean toward a UI that avoids horizontal scroll bars.
It becomes a usability issue because the horz scroll bar over lays textual information
that a person may try to copy and paste. (This is an issue with the table view itself that I was planning on addressing soon)

@Talkless
Copy link

I agree with @jonatack, flexible approach would be better IMO, i.e. without "forced" newlines.

@RandyMcMillan
Copy link
Contributor Author

I agree with @jonatack and @Talkless
I just wanted to provide examples to prompt discussion on these nuanced decisions. :)

src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
src/qt/guiutil.cpp Outdated Show resolved Hide resolved
@hebasto hebasto added UI All about "look and feel" and removed Design labels May 9, 2021
@hebasto
Copy link
Member

hebasto commented May 9, 2021

fb5e880 -- looking at the diff lines count I see +7/-3. If drop unrelated changes, it could be +4/-1.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK a0f7978, tested on Linux Mint 20.1 (Qt 5.12.8):

Node A:

Screenshot from 2021-05-10 10-04-19
Screenshot from 2021-05-10 10-04-53

Node B:

Screenshot from 2021-05-10 10-05-46
Screenshot from 2021-05-10 10-06-16

@Talkless
Copy link

tACK a0f7978 on same environment as previously.

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

re-ACK a0f7978. Tested under Gentoo Linux with Xfce4 (Qt 5.15.2).

@hebasto hebasto merged commit 4bc3b16 into bitcoin-core:master May 10, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 11, 2021
a0f7978 qt: enable wordWrap for peers-tab detail services (randymcmillan)

Pull request description:

  Enable wordWrap for peers-tab detailView Services

ACKs for top commit:
  Talkless:
    tACK a0f7978 on same environment as previously.
  hebasto:
    ACK a0f7978, tested on Linux Mint 20.1 (Qt 5.12.8):
  kristapsk:
    re-ACK a0f7978. Tested under Gentoo Linux with Xfce4 (Qt 5.15.2).

Tree-SHA512: 872e511d2ecfa72fea0fd3284a958b45ee8aee138469ce7f9cd853cd9098b9583917909934b0a5c96f9b81ea1567bcea6a037558829bb79f2a3f413a83df06e6
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 20, 2021
a0f7978 qt: enable wordWrap for peers-tab detail services (randymcmillan)

Pull request description:

  Enable wordWrap for peers-tab detailView Services

ACKs for top commit:
  Talkless:
    tACK a0f7978 on same environment as previously.
  hebasto:
    ACK a0f7978, tested on Linux Mint 20.1 (Qt 5.12.8):
  kristapsk:
    re-ACK a0f7978. Tested under Gentoo Linux with Xfce4 (Qt 5.15.2).

Tree-SHA512: 872e511d2ecfa72fea0fd3284a958b45ee8aee138469ce7f9cd853cd9098b9583917909934b0a5c96f9b81ea1567bcea6a037558829bb79f2a3f413a83df06e6
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 21, 2021
a0f7978 qt: enable wordWrap for peers-tab detail services (randymcmillan)

Pull request description:

  Enable wordWrap for peers-tab detailView Services

ACKs for top commit:
  Talkless:
    tACK a0f7978 on same environment as previously.
  hebasto:
    ACK a0f7978, tested on Linux Mint 20.1 (Qt 5.12.8):
  kristapsk:
    re-ACK a0f7978. Tested under Gentoo Linux with Xfce4 (Qt 5.15.2).

Tree-SHA512: 872e511d2ecfa72fea0fd3284a958b45ee8aee138469ce7f9cd853cd9098b9583917909934b0a5c96f9b81ea1567bcea6a037558829bb79f2a3f413a83df06e6
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
a0f7978 qt: enable wordWrap for peers-tab detail services (randymcmillan)

Pull request description:

  Enable wordWrap for peers-tab detailView Services

ACKs for top commit:
  Talkless:
    tACK a0f7978 on same environment as previously.
  hebasto:
    ACK a0f7978, tested on Linux Mint 20.1 (Qt 5.12.8):
  kristapsk:
    re-ACK a0f7978. Tested under Gentoo Linux with Xfce4 (Qt 5.15.2).

Tree-SHA512: 872e511d2ecfa72fea0fd3284a958b45ee8aee138469ce7f9cd853cd9098b9583917909934b0a5c96f9b81ea1567bcea6a037558829bb79f2a3f413a83df06e6
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 9, 2022
a0f7978 qt: enable wordWrap for peers-tab detail services (randymcmillan)

Pull request description:

  Enable wordWrap for peers-tab detailView Services

ACKs for top commit:
  Talkless:
    tACK a0f7978 on same environment as previously.
  hebasto:
    ACK a0f7978, tested on Linux Mint 20.1 (Qt 5.12.8):
  kristapsk:
    re-ACK a0f7978. Tested under Gentoo Linux with Xfce4 (Qt 5.15.2).

Tree-SHA512: 872e511d2ecfa72fea0fd3284a958b45ee8aee138469ce7f9cd853cd9098b9583917909934b0a5c96f9b81ea1567bcea6a037558829bb79f2a3f413a83df06e6
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@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.

None yet

6 participants