Fix transaction view/table #662

luke-jr · 2022-09-06T17:51:22Z

#204 reverted a necessary bugfix, and #205 introduced regressions since setModel resets column widths. Note that you need to delete your saved GUI config to see the fix, otherwise the prior widths are restored.

Before regressions:

After regressions / current master:

With this PR:

DrahtBot · 2022-09-06T21:20:01Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#684 (Improve 'Requested Payments History' Multiselect by john-moffett)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jarolrod

if we revert the PR's aren't we still left with a buggy transaction table? Additionally, it's possible that the class mentioned can lead to a segfault when loading in a very large wallet:

luke-jr · 2022-09-19T13:35:04Z

if we revert the PR's aren't we still left with a buggy transaction table?

I'm not aware of any documented bugs. With the PRs reverted, it works perfect for me.

Additionally, it's possible that the class mentioned can lead to a segfault when loading in a very large wallet:

These bugs seem outside the scope of this PR, but if you can explain, I can try to take a shot at fixing them?

hebasto · 2022-12-06T18:07:35Z

From PR description it is not clear what actual bug this PR is supposed to fix.

#204 reverted a necessary bugfix,

Wrong. See #204 PR description.

and #205 introduced regressions since setModel resets column widths.

Wrong. Column widths belong to a "View" layer. A "Model" layer should do nothing with a visual representation. It is up to user to set the "View" layer, including column widths.

luke-jr · 2022-12-06T23:27:12Z

Not wrong. See screenshots.

hebasto · 2023-05-31T16:43:55Z

Closing due to a long period of inactivity here. Feel free to reopen.

…lumn width and stretch on last section

This reverts commit 788205c.

This reverts commit f5c8093.

This reverts commit 3913d1e.

achow101 · 2023-06-29T22:59:35Z

Reopening was requested.

luke-jr · 2023-06-29T23:00:12Z

Re-confirmed bug still exists, and rebased this fix.

hebasto · 2023-07-03T14:50:00Z

Tested 8e428cc on Ubuntu 23.04.

Application window resizing leads to:

when the content of the last column "Amount" goes out of the scope.

maflcko · 2023-09-06T08:08:35Z

From CI:

./qt/receivecoinsdialog.h:54:48: error: use default member initializer for 'columnResizingFixer' [modernize-use-default-member-init,-warnings-as-errors]
    GUIUtil::TableViewLastColumnResizingFixer *columnResizingFixer;
                                               ^
                                                                  {nullptr}

pablomartin4btc

Tested 8e428cc on Ubuntu 22.04.

Deleted TransactionViewHeaderState row manually from qt config file as requested in the description by the author (.config/Bitcoin/Bitcoin-Qt-regtest.conf), instead of using -resetguisettings as I didn't want to lose the rest.

As @hebasto posted above, "Amount" column goes out of scope when I resize to the right and then to the left (and the same happens with the "Requested payments history" in the receivecoinsdialog).

Something I noticed when I resize the Label column enough to fit the most of the address (when are not labeled) is that with this PR changes, when the "Amount" column is correctly resized and gets fit in the table view, it's changing the Label column size that I set manually, so that's a bit contradictory, we let the user change something and then we correct the user:

So, in terms of the 2 issues detected and explained above I prefer the current state of the table view, in master we also have the horizontal scrollbar at the bottom which has disappear with the changes of this PR.

Going thru all the PRs and beyond, is the actual issue that the "last column itself cannot be resized" (in the case of the transaction view/ table would be the "Amount" column)? In this case I see the issue is still in master (user can make it wider but not shrink it - this is because of setStretchLastSection(true)) and this PR doesn't fix it, the PR deals more with the resizing of the table but user can't make the last column shorter or larger (last column is set as "interactive" but it's fixed for the user perspective). I see where this is achieved is on the Peers table (setStretchLastSection is not called so it's false by default), user can set the last column "User Agent" width freely.

I can verify that there's another issue in master, and this PR solves, which is the fact that the default widths are not set properly the first time qt runs on a machine (or settings of the transaction view are missing from the qt config file) and user can see the 2nd. screenshot from the top description. As it's mentioned in the description as well "setModel resets column widths", can't we not move the settings state restore/ set column widths defaults at the very end of it?.

Perhaps the description of the PR could be clearer in terms of what the proper issue is and just mention the history of tries as a reference. This is original issue in the bitcoin repo: #2862.

pablomartin4btc · 2023-09-16T18:43:22Z

src/qt/guiutil.cpp

+    }
+}
+
+// When the table's geometry is ready, we manually perform the stretch of the "Message" column,


What the "Message" column would be? Sounds like something dynamic but it's fixed to the secondToLastColumIndex (which is the Label column in the transactions table).

pablomartin4btc · 2023-09-16T22:12:43Z

src/qt/guiutil.cpp

+    secondToLastColumnIndex = columnCount - 2;
+    tableView->horizontalHeader()->setMinimumSectionSize(allColumnsMinimumWidth);
+    setViewHeaderResizeMode(secondToLastColumnIndex, QHeaderView::Interactive);
+    setViewHeaderResizeMode(lastColumnIndex, QHeaderView::Interactive);


This line doesn't make any difference, user still can't change the last column size manually.

Suggested change

setViewHeaderResizeMode(lastColumnIndex, QHeaderView::Interactive);

DrahtBot · 2024-02-05T14:32:57Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

hebasto · 2024-02-11T22:05:41Z

Closing due to lack of support from the PR author (unaddressed comments, the failed CI).

DrahtBot mentioned this pull request Sep 6, 2022

Bugfix: When restoring table columns, still set their minimum column width and stretch on last section #368

Closed

jarolrod reviewed Sep 19, 2022

View reviewed changes

hebasto added the UI All about "look and feel" label Oct 26, 2022

bitcoin-core deleted a comment from DrahtBot Nov 7, 2022

DrahtBot mentioned this pull request Dec 6, 2022

Improve 'Requested Payments History' Multiselect #684

Open

This was referenced Dec 16, 2022

clang-tidy: Fix modernize-use-default-member-init in headers and force to check all headers bitcoin/bitcoin#26705

Merged

clang-tidy: Force checks for headers in src/qt #686

Merged

DrahtBot added the Needs rebase label Jan 17, 2023

hebasto closed this May 31, 2023

luke-jr added 4 commits June 29, 2023 22:47

Bugfix: GUI: When restoring table columns, still set their minimum co…

e9f3b5f

…lumn width and stretch on last section

Revert "qt: Move transactionView properties settings to constructor"

a3dbdda

This reverts commit 788205c.

Revert "qt: Move recentRequestsView properties settings to constructor"

379728e

This reverts commit f5c8093.

Revert "qt: Drop buggy TableViewLastColumnResizingFixer class"

8e428cc

This reverts commit 3913d1e.

achow101 reopened this Jun 29, 2023

luke-jr force-pushed the qt_fix_txview_202209 branch from e515b0f to 8e428cc Compare June 29, 2023 22:59

DrahtBot added CI failed and removed Needs rebase labels Jun 30, 2023

pablomartin4btc reviewed Sep 16, 2023

View reviewed changes

hebasto closed this Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix transaction view/table #662

Fix transaction view/table #662

luke-jr commented Sep 6, 2022

DrahtBot commented Sep 6, 2022 •

edited

Loading

jarolrod left a comment

luke-jr commented Sep 19, 2022

hebasto commented Dec 6, 2022

luke-jr commented Dec 6, 2022

hebasto commented May 31, 2023

achow101 commented Jun 29, 2023

luke-jr commented Jun 29, 2023

hebasto commented Jul 3, 2023

maflcko commented Sep 6, 2023

pablomartin4btc left a comment •

edited

Loading

pablomartin4btc Sep 16, 2023

pablomartin4btc Sep 16, 2023

DrahtBot commented Feb 5, 2024

hebasto commented Feb 11, 2024

Fix transaction view/table #662

Fix transaction view/table #662

Conversation

luke-jr commented Sep 6, 2022

DrahtBot commented Sep 6, 2022 • edited Loading

Reviews

Conflicts

jarolrod left a comment

Choose a reason for hiding this comment

luke-jr commented Sep 19, 2022

hebasto commented Dec 6, 2022

luke-jr commented Dec 6, 2022

hebasto commented May 31, 2023

achow101 commented Jun 29, 2023

luke-jr commented Jun 29, 2023

hebasto commented Jul 3, 2023

maflcko commented Sep 6, 2023

pablomartin4btc left a comment • edited Loading

Choose a reason for hiding this comment

pablomartin4btc Sep 16, 2023

Choose a reason for hiding this comment

pablomartin4btc Sep 16, 2023

Choose a reason for hiding this comment

DrahtBot commented Feb 5, 2024

hebasto commented Feb 11, 2024

DrahtBot commented Sep 6, 2022 •

edited

Loading

pablomartin4btc left a comment •

edited

Loading