refactor: Use enum type as switch argument in *TableModel #166

1. 
   hebasto commented at 8:59 pm on December 25, 2020: member

   This PR makes code more maintainable by leveraging -Wswitch compiler warnings.

   Only the RecentRequestsTableModel is not refactored, because its enum ColumnIndex contains additional NUMBER_OF_COLUMNS value.

   No behavior change.

2. DrahtBot added the label Needs rebase on Dec 28, 2020
3. hebasto force-pushed on Dec 30, 2020
4. 
   hebasto commented at 6:42 pm on December 30, 2020: member

   Rebased aeb11d3704faa7c2cc71f4e23462e5dcd050c6df -> dae033e3fd5c910e961f5f5553004710fb27dd43 (pr166.01 -> pr166.02) due to the conflict with #162.
5. DrahtBot removed the label Needs rebase on Dec 30, 2020
6. DrahtBot added the label Needs rebase on Jan 11, 2021
7. hebasto force-pushed on Jan 11, 2021
8. 
   hebasto commented at 10:09 am on January 11, 2021: member

   Rebased dae033e3fd5c910e961f5f5553004710fb27dd43 -> d4508f1e82b2bbf591af2cad25933d5050c404bb (pr166.02 -> pr166.03) due to the conflict with #161.
9. DrahtBot removed the label Needs rebase on Jan 11, 2021
10. 
    promag commented at 12:40 pm on January 11, 2021: contributor

    Concept ACK.

    It’s not clear to me if data() implementation should check for out of bounds. I think Qt views and proxy model behave correctly by querying data inside columnCount() and rowCount().

    As follow up:

    • move AddressTableModel::columns member to priv instance
    • drop section < columns.size() check in AddressTableModel::headerData
11. 
    hebasto commented at 7:57 pm on February 21, 2021: member

    @promag

    It’s not clear to me if data() implementation should check for out of bounds. I think Qt views and proxy model behave correctly by querying data inside columnCount() and rowCount().

    At least, these changes do not affect the current behavior, right?

12. 
    hebasto commented at 7:59 pm on February 21, 2021: member

    @jonasschnelli Maybe tag this pr with 22.0 milestone?
13. DrahtBot added the label Needs rebase on Feb 22, 2021
14. qt, refactor: Use enum type as switch argument in AddressTableModel ab8a747d1c
15. qt, refactor: Use enum type as switch argument in BanTableModel a35223f1cd
16. qt, refactor: Use enum type as switch argument in PeerTableModel 52f122c11f
17. qt, refactor: Use enum type as switch argument in TransactionTableModel 1d5d832d5c
18. hebasto force-pushed on Feb 22, 2021
19. 
    hebasto commented at 7:40 am on February 22, 2021: member

    Rebased d4508f1e82b2bbf591af2cad25933d5050c404bb -> 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182 (pr166.03 -> pr166.04) due to the conflict with #179.
20. DrahtBot removed the label Needs rebase on Feb 22, 2021
21. 
    jarolrod commented at 8:01 pm on February 22, 2021: member

    ACK 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182, tested on macOS 11.1 Qt 5.15.2

    This refactor brings a safer switch statement design to the address, ban, peer, and transaction table models. It also brings us in accordance with the switch design recommendation included in developer-notes.md.

    The ability to leverage the -Wswitch compiler warning, as mentioned in the OP, is another benefit of this refactor. An example of the benefit this brings can be shown by considering a scenario where a new Address type is introduced, and the author forgets to update the code to consider the new type. In such a scenario, the compiler will warn about this, thus preventing errors.

22. 
    leonardojobim commented at 9:46 am on February 26, 2021: none

    ACK https://github.com/bitcoin-core/gui/pull/166/commits/1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182, tested on Ubuntu 20.04 Qt 5.12.8
23. 
    promag commented at 4:16 pm on February 26, 2021: contributor

    Code review ACK 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182.
24. 
    hebasto commented at 4:26 pm on February 26, 2021: member

    @leonardojobim

    ACK ab8a747, tested on Ubuntu 20.04 Qt 5.12.8

    Thank you for your review!

    Do you mind mentioning the top pr commit with your ACK, i.e., 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182, not ab8a747d1ced9f20ca32f9898418be70670da71a?

25. 
    leonardojobim commented at 10:34 pm on February 26, 2021: none

    No problem. Changed it to the top PR commit.
26. hebasto added the label Refactoring on Mar 6, 2021
27. MarcoFalke merged this on Mar 7, 2021
28. MarcoFalke closed this on Mar 7, 2021

    ---

29. hebasto deleted the branch on Mar 7, 2021
30. sidhujag referenced this in commit 81130fd6a9 on Mar 7, 2021
31. gwillen referenced this in commit 2a4f512a08 on Jun 28, 2022
32. bitcoin-core locked this on Aug 16, 2022

Contributors
hebasto promag jarolrod leonardojobim

Labels
Refactoring