Enable wordWrap for Services

RandyMcMillan commented at 11:33 pm on April 24, 2021: contributor

Enable wordWrap for peers-tab detailView Services

RandyMcMillan commented at 0:02 am on April 25, 2021: contributor

Blanks instead:

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

Talkless approved

Talkless commented at 12:22 pm on April 25, 2021: none

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 added the label Design on Apr 25, 2021

jonatack commented at 1:03 pm on April 25, 2021: contributor

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

kristapsk approved

kristapsk commented at 1:15 pm on April 25, 2021: contributor

ACK 84612fad4585b4fd6b12b37b9c7349958bb4362d. I agree that current “&” separator is ok.

jonatack commented at 2:05 pm on April 25, 2021: contributor

ACK 84612fad4585b4fd6b12b37b9c7349958bb4362d

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 commented at 3:15 pm on April 25, 2021: contributor

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

jonatack commented at 3:46 pm on April 25, 2021: 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 commented at 4:09 pm on April 25, 2021: member

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:

 0--- a/src/qt/guiutil.cpp
 1+++ b/src/qt/guiutil.cpp
 2@@ -708,7 +708,7 @@ QString formatServicesStr(quint64 mask)
 3     }
 4 
 5     if (strList.size())
 6-        return strList.join(" & ");
 7+        return strList.join("\n");
 8     else
 9         return QObject::tr("None");
10 }

?

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 commented at 4:15 pm on April 25, 2021: member

@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 renamed this:
~~peers-tab: enable wordWrap for Services~~
qt: enable wordWrap for Services
on Apr 25, 2021

hebasto approved

hebasto commented at 4:40 pm on April 25, 2021: member

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

DeepinScreenshot_select-area_20210425193959

hebasto renamed this:
~~qt: enable wordWrap for Services~~
Enable wordWrap for Services
on Apr 25, 2021

jonatack commented at 4:53 pm on April 25, 2021: contributor

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

hebasto commented at 4:55 pm on April 25, 2021: member

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

Right. Is it bad?

kristapsk approved

kristapsk commented at 5:03 pm on April 25, 2021: contributor

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

jonatack commented at 5:08 pm on April 25, 2021: 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.

jarolrod commented at 5:08 pm on April 25, 2021: member

ACK bd04efe84069cdbee4f4b80150613a3bec26dda1

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

in src/qt/forms/debugwindow.ui:1209 in bd04efe840 outdated

1197@@ -1198,6 +1198,9 @@
1198                  <property name="textFormat">
1199                   <enum>Qt::PlainText</enum>
1200                  </property>
1201+                 <property name="wordWrap">
1202+                  <bool>true</bool>
1203+                 </property>

jonatack commented at 5:11 pm on April 25, 2021:

Is this still needed if we hardcode it to use one line per flag?

hebasto commented at 5:18 pm on April 25, 2021:

I think, @jonatack is right, and this change seems redundant.

RandyMcMillan commented at 8:07 pm on May 7, 2021:

This was referring to the force new line version. Since we are concluding that forcing a new line is not preferred we should leave it.

hebasto commented at 5:14 pm on April 25, 2021: member

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 commented at 5:15 pm on April 25, 2021: contributor

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 commented at 5:30 pm on April 25, 2021: contributor

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 commented at 5:31 pm on April 25, 2021: member

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 commented at 5:36 pm on April 25, 2021: contributor

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 commented at 5:49 pm on April 26, 2021: none

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

RandyMcMillan commented at 1:39 pm on May 7, 2021: contributor

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

in src/qt/guiutil.cpp:460 in c2028638c4 outdated

453@@ -455,8 +454,8 @@ bool ToolTipToRichTextFilter::eventFilter(QObject *obj, QEvent *evt)
454     return QObject::eventFilter(obj, evt);
455 }
456 
457-LabelOutOfFocusEventFilter::LabelOutOfFocusEventFilter(QObject* parent)
458-    : QObject(parent)
459+LabelOutOfFocusEventFilter::LabelOutOfFocusEventFilter(QObject* parent) :
460+    QObject(parent)

hebasto commented at 5:38 pm on May 9, 2021:

This change looks unrelated to this PR.

in src/qt/guiutil.cpp:437 in c2028638c4 outdated

433@@ -434,7 +434,6 @@ ToolTipToRichTextFilter::ToolTipToRichTextFilter(int _size_threshold, QObject *p
434     QObject(parent),
435     size_threshold(_size_threshold)
436 {
437-

hebasto commented at 5:38 pm on May 9, 2021:

This change looks unrelated to this PR.

in src/qt/guiutil.cpp:710 in c2028638c4 outdated

706@@ -708,7 +707,7 @@ QString formatServicesStr(quint64 mask)
707     }
708 
709     if (strList.size())
710-        return strList.join(" & ");
711+        return strList.join(" ");

hebasto commented at 5:39 pm on May 9, 2021:

I agree with @jonatack and @Talkless

I have an impression that @jonatack’ suggestion is:

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

hebasto changes_requested

hebasto removed the label Design on May 9, 2021

hebasto added the label UI on May 9, 2021

hebasto commented at 9:45 pm on May 9, 2021: member

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

qt: enable wordWrap for peers-tab detail services a0f7978674

RandyMcMillan requested review from hebasto on May 10, 2021

RandyMcMillan requested review from jonatack on May 10, 2021

hebasto approved

hebasto commented at 7:09 am on May 10, 2021: member

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

Node A:

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

Node B:

Screenshot from 2021-05-10 10-05-46

Talkless commented at 5:46 pm on May 10, 2021: none

tACK a0f797867433b0d3e8d7851ddf05743fe70d320a on same environment as previously.

kristapsk approved

kristapsk commented at 9:20 pm on May 10, 2021: contributor

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

hebasto merged this on May 10, 2021

hebasto closed this on May 10, 2021

sidhujag referenced this in commit 3bf004dc81 on May 11, 2021

PastaPastaPasta referenced this in commit 5df7dce84e on Oct 20, 2021

PastaPastaPasta referenced this in commit 1e39067b3d on Oct 21, 2021

pravblockc referenced this in commit c135639375 on Nov 18, 2021

gades referenced this in commit 4410b6196c on May 9, 2022

gwillen referenced this in commit 8d570c2dcb on Jun 1, 2022

bitcoin-core locked this on Aug 16, 2022

Enable wordWrap for Services #293

Blanks instead: