Fix transaction view/table #662
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, 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. |
I'm not aware of any documented bugs. With the PRs reverted, it works perfect for me.
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.
Wrong. See #204 PR description.
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. |
|
Not wrong. See screenshots. |
|
Closing due to a long period of inactivity here. Feel free to reopen. |
|
Reopening was requested. |
e515b0f to
8e428cc
Compare
|
Re-confirmed bug still exists, and rebased this fix. |
|
Tested 8e428cc on Ubuntu 23.04. Application window resizing leads to: when the content of the last column "Amount" goes out of the scope. |
|
From CI: |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| // When the table's geometry is ready, we manually perform the stretch of the "Message" column, |
There was a problem hiding this comment.
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).
| secondToLastColumnIndex = columnCount - 2; | ||
| tableView->horizontalHeader()->setMinimumSectionSize(allColumnsMinimumWidth); | ||
| setViewHeaderResizeMode(secondToLastColumnIndex, QHeaderView::Interactive); | ||
| setViewHeaderResizeMode(lastColumnIndex, QHeaderView::Interactive); |
There was a problem hiding this comment.
This line doesn't make any difference, user still can't change the last column size manually.
| setViewHeaderResizeMode(lastColumnIndex, QHeaderView::Interactive); |
|
🤔 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. |
|
Closing due to lack of support from the PR author (unaddressed comments, the failed CI). |
#204 reverted a necessary bugfix, and #205 introduced regressions since
setModelresets 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: