Fix transaction view/table #662

pull luke-jr wants to merge 4 commits into bitcoin-core:master from luke-jr:qt_fix_txview_202209 changing 6 files +211 −32
  1. luke-jr commented at 5:51 pm on September 6, 2022: member

    #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:

    Screenshot_20220905_232835 branch-21

    After regressions / current master:

    Screenshot_20220905_233054 branch-22

    With this PR:

    Screenshot_20220906_134210

  2. DrahtBot commented at 9:20 pm on September 6, 2022: contributor

    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.

  3. DrahtBot cross-referenced this on Sep 6, 2022 from issue Bugfix: When restoring table columns, still set their minimum column width and stretch on last section by luke-jr
  4. jarolrod commented at 3:50 am on September 19, 2022: member
    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:
  5. luke-jr commented at 1:35 pm on September 19, 2022: member

    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?

  6. hebasto added the label UI on Oct 26, 2022
  7. bitcoin-core deleted a comment on Nov 7, 2022
  8. DrahtBot cross-referenced this on Dec 6, 2022 from issue Improve 'Requested Payments History' Multiselect by john-moffett
  9. hebasto commented at 6:07 pm on December 6, 2022: member

    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.

  10. luke-jr commented at 11:27 pm on December 6, 2022: member
    Not wrong. See screenshots.
  11. DrahtBot cross-referenced this on Dec 16, 2022 from issue clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers by hebasto
  12. DrahtBot cross-referenced this on Dec 17, 2022 from issue clang-tidy: Force checks for headers in `src/qt` by hebasto
  13. DrahtBot added the label Needs rebase on Jan 17, 2023
  14. hebasto commented at 4:43 pm on May 31, 2023: member
    Closing due to a long period of inactivity here. Feel free to reopen.
  15. hebasto closed this on May 31, 2023

  16. Bugfix: GUI: When restoring table columns, still set their minimum column width and stretch on last section e9f3b5ff6d
  17. Revert "qt: Move transactionView properties settings to constructor"
    This reverts commit 788205c3f783fb20bfdfd403be6befba149772ca.
    a3dbdda922
  18. Revert "qt: Move recentRequestsView properties settings to constructor"
    This reverts commit f5c8093e778069f4930a84452398c339aad58e79.
    379728eeab
  19. Revert "qt: Drop buggy TableViewLastColumnResizingFixer class"
    This reverts commit 3913d1e8c1f604bdd622d5e81e5077ef52b30466.
    8e428cc118
  20. achow101 commented at 10:59 pm on June 29, 2023: member
    Reopening was requested.
  21. achow101 reopened this on Jun 29, 2023

  22. luke-jr force-pushed on Jun 29, 2023
  23. luke-jr commented at 11:00 pm on June 29, 2023: member
    Re-confirmed bug still exists, and rebased this fix.
  24. DrahtBot removed the label Needs rebase on Jun 30, 2023
  25. DrahtBot added the label CI failed on Jun 30, 2023
  26. hebasto commented at 2:50 pm on July 3, 2023: member

    Tested 8e428cc1188568aaeeaeb8f1f4498ae5b738a5e2 on Ubuntu 23.04.

    Application window resizing leads to:

    image

    when the content of the last column “Amount” goes out of the scope.

  27. maflcko commented at 8:08 am on September 6, 2023: contributor

    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}
    
  28. in src/qt/guiutil.cpp:586 in 8e428cc118
    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,
    


    pablomartin4btc commented at 6:43 pm on September 16, 2023:
    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).
  29. pablomartin4btc commented at 7:28 pm on September 16, 2023: contributor

    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). Peek 2023-09-16 14-39

    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: Peek 2023-09-16 15-31

    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.

  30. in src/qt/guiutil.cpp:613 in 8e428cc118
    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);
    


    pablomartin4btc commented at 10:12 pm on September 16, 2023:

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

  31. DrahtBot commented at 2:32 pm on February 5, 2024: contributor

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

  32. hebasto commented at 10:05 pm on February 11, 2024: member
    Closing due to lack of support from the PR author (unaddressed comments, the failed CI).
  33. hebasto closed this on Feb 11, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 20:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me