The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
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.
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?
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
setModelresets 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.
This reverts commit 788205c3f783fb20bfdfd403be6befba149772ca.
This reverts commit f5c8093e778069f4930a84452398c339aad58e79.
This reverts commit 3913d1e8c1f604bdd622d5e81e5077ef52b30466.
Tested 8e428cc1188568aaeeaeb8f1f4498ae5b738a5e2 on Ubuntu 23.04.
Application window resizing leads to:
when the content of the last column “Amount” goes out of the scope.
From CI:
0./qt/receivecoinsdialog.h:54:48: error: use default member initializer for 'columnResizingFixer' [modernize-use-default-member-init,-warnings-as-errors]
1 GUIUtil::TableViewLastColumnResizingFixer *columnResizingFixer;
2 ^
3 {nullptr}
581+ {
582+ resizeColumn(logicalIndex, remainingWidth);
583+ }
584+}
585+
586+// When the table's geometry is ready, we manually perform the stretch of the "Message" column,
secondToLastColumIndex (which is the Label column in the transactions table).
Tested 8e428cc1188568aaeeaeb8f1f4498ae5b738a5e2 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.
608+ columnCount = tableView->horizontalHeader()->count();
609+ lastColumnIndex = columnCount - 1;
610+ secondToLastColumnIndex = columnCount - 2;
611+ tableView->horizontalHeader()->setMinimumSectionSize(allColumnsMinimumWidth);
612+ setViewHeaderResizeMode(secondToLastColumnIndex, QHeaderView::Interactive);
613+ setViewHeaderResizeMode(lastColumnIndex, QHeaderView::Interactive);
This line doesn’t make any difference, user still can’t change the last column size manually.
🤔 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.