bisq icon indicating copy to clipboard operation
bisq copied to clipboard

Remove GUIUtil.setFitToRowsForTableView();

Open ManfredKarrer opened this issue 5 years ago • 4 comments

The utility method GUIUtil.setFitToRowsForTableView(); does not work well as it caused those not updated views for unknown reasons. In ProposalsView @ripcurlx has used another approach with a listener on the stage height to adjust the table height. We should apply the same concept to all the other views where setFitToRowsForTableView is still used.

ManfredKarrer avatar Mar 29 '19 21:03 ManfredKarrer

As a first step I literally removed it from source code. I probably need some assistance to move this topic forward because I don't fully understand what was it's purpose. I also wonder if usage of this method was necessary on all affected views (listed in pr description).

(CC: @ripcurlx, @chimp1984)

ghost avatar Jan 28 '22 12:01 ghost

What is not not clear with that code?

 public static void setFitToRowsForTableView(TableView<?> tableView,
                                                int rowHeight,
                                                int headerHeight,
                                                int minNumRows,
                                                int maxNumRows) {
        int size = tableView.getItems().size();
        int minHeight = rowHeight * minNumRows + headerHeight;
        int maxHeight = rowHeight * maxNumRows + headerHeight;
        checkArgument(maxHeight >= minHeight, "maxHeight cannot be smaller as minHeight");
        int height = Math.min(maxHeight, Math.max(minHeight, size * rowHeight + headerHeight));

        tableView.setPrefHeight(-1);
        tableView.setVisible(false);
        // We need to delay the setter to the next render frame as otherwise views don' get updated in some cases
        // Not 100% clear what causes that issue, but seems the requestLayout method is not called otherwise.
        // We still need to set the height immediately, otherwise some views render an incorrect layout.
        tableView.setPrefHeight(height);

        UserThread.execute(() -> {
            tableView.setPrefHeight(height);
            tableView.setVisible(true);
        });
    }

Also the name suggests whats the intention. That the table hight is adjusted to the number of rows.

Its not a good attempt to just remove code when you are not clear about the functionality and intention behind. The only problem with the current solution is that it seems to not work that reliable because of JavaFX framework specific rendering issues. Have not observed it if it is even still an issue. Maybe that GH issue is outdated.

chimp1984 avatar Jan 28 '22 13:01 chimp1984

What is not not clear with that code?

Nothing - just didn't spent enough time to analyze why we need it at all - most tables in app doesn't uses it. At first glance I didn't observed anything wrong with UI after removing it but I'll try to check it out better.


Its not a good attempt to just remove code when you are not clear about the functionality and intention behind.

Sometimes it's a good idea to throw away old toys to make room for new ones.

ghost avatar Jan 28 '22 15:01 ghost

Have not observed it if it is even still an issue. Maybe that GH issue is outdated.

OK, I'm gonna leave work on it for now.

ghost avatar Jan 31 '22 09:01 ghost