Implement better scaling of main table showing entries #7181
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| package org.jabref.gui.maintable; | ||
|
|
||
| import java.lang.reflect.Field; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
| import javafx.scene.control.TableColumn; | ||
| import javafx.scene.control.TableColumnBase; | ||
| import javafx.scene.control.TableView; | ||
| import javafx.util.Callback; | ||
|
|
@@ -22,75 +23,158 @@ public class SmartConstrainedResizePolicy implements Callback<TableView.ResizeFe | |
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(SmartConstrainedResizePolicy.class); | ||
|
|
||
| private Map<TableColumnBase, Double> expansionShare = new HashMap<>(); | ||
|
|
||
| @Override | ||
| public Boolean call(TableView.ResizeFeatures prop) { | ||
| if (prop.getColumn() == null) { | ||
| // happens at initialization and at window resize | ||
|
There was a problem hiding this comment. Since you below call I think the easiest solution is to use There was a problem hiding this comment. We argue that the preferred size is the size the user set manually. It is difficult to distinguish whether a preferred size was set by JabRef automatically or by the user. During experiments with the functionality, it seems that maximization of the window and shrinkage and maximization and so on did not change the columns alignment. Thus, we opted to store the currently stored ratios. For instance, that way when a user resizes the field "title" to twice the size of "author", this ratio is kept when resizing the window. There was a problem hiding this comment. Yeah, I was worried that you re-calculate the preferred width (e.g. in the initialization code), so this not necessarily need to correspond to the user's wish. But if that's not a problem, then nice. |
||
| LOGGER.debug("Table is fully rendered"); | ||
| // This way, the proportions of the columns are kept during resize of the window | ||
| determineWidthShare(prop.getTable()); | ||
| return rearrangeColumns(prop.getTable()); | ||
| } else { | ||
| return constrainedResize(prop); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Determines the share of the total width each column has | ||
| */ | ||
| private void determineWidthShare(TableView table) { | ||
| // We need to store the initial preferred width, because "setWidth()" does not exist | ||
| // There is only "setMinWidth", "setMaxWidth", and "setPrefWidth | ||
|
|
||
| Double totalInitialPreferredWidths; | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| List<? extends TableColumnBase<?, ?>> visibleLeafColumns = table.getVisibleLeafColumns(); | ||
| totalInitialPreferredWidths = visibleLeafColumns.stream() | ||
| .filter(TableColumnBase::isResizable) | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| .mapToDouble(TableColumnBase::getPrefWidth).sum(); | ||
| for (TableColumnBase<?, ?> column : visibleLeafColumns) { | ||
| if (column.isResizable()) { | ||
| expansionShare.put(column, column.getPrefWidth() / totalInitialPreferredWidths); | ||
| } else { | ||
| expansionShare.put(column, 0d); | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Determines the new width of the column based on the requested delta. It respects the min and max width of the column. | ||
| * | ||
| * @param column The column the resize is requested | ||
| * @param delta The delta requested | ||
| * @return the new size, Optional.empty() if no resize is possible | ||
| */ | ||
| private Optional<Double> determineNewWidth(TableColumn<?, ?> column, Double delta) { | ||
| // This is com.sun.javafx.scene.control.skin.Utils.boundedSize with more comments and Optionals | ||
|
|
||
| // Calculate newWidth based on delta and constraint of the column | ||
| double oldWidth = column.getWidth(); | ||
| double newWidth; | ||
| if (delta < 0) { | ||
| double minWidth = column.getMinWidth(); | ||
| LOGGER.trace("MinWidth {}", minWidth); | ||
| newWidth = Math.max(minWidth, oldWidth + delta); | ||
| } else { | ||
| double maxWidth = column.getMaxWidth(); | ||
| LOGGER.trace("MaxWidth {}", maxWidth); | ||
| newWidth = Math.min(maxWidth, oldWidth + delta); | ||
| } | ||
| LOGGER.trace("Size: {} -> {}", oldWidth, newWidth); | ||
| if (oldWidth == newWidth) { | ||
| return Optional.empty(); | ||
| } | ||
| return Optional.of(newWidth); | ||
| } | ||
|
|
||
| /** | ||
| * Resizes the table based on the content. The main driver is that if the content might fit into the table without horizontal scroll bar. | ||
| * In case the content fitted before the resize and will fit afterwards, the delta is distributed among the remaining columns - instead of just moving the columns right of the current column. | ||
| * In case the content does not fit anymore, a horizontal scroll bar is shown. | ||
| * In all cases the minimum/maximum width of each column is respected. | ||
| */ | ||
| private Boolean constrainedResize(TableView.ResizeFeatures<?> prop) { | ||
| TableView<?> table = prop.getTable(); | ||
| Double tableWidth = getContentWidth(table); | ||
| List<? extends TableColumnBase<?, ?>> visibleLeafColumns = table.getVisibleLeafColumns(); | ||
| Double requiredWidth = visibleLeafColumns.stream().mapToDouble(TableColumnBase::getWidth).sum(); | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Double delta = prop.getDelta(); | ||
|
|
||
| TableColumn<?, ?> userChosenColumnToResize = prop.getColumn(); | ||
|
|
||
| boolean columnsCanFitTable = requiredWidth + delta < tableWidth; | ||
| if (columnsCanFitTable) { | ||
| LOGGER.debug("User shrunk column in that way thus that window can contain all the columns"); | ||
| if (requiredWidth >= tableWidth) { | ||
| LOGGER.debug("Before, the content did not fit. Rearrange everything."); | ||
| return rearrangeColumns(table); | ||
| } | ||
| LOGGER.debug("Everything already fit. We distribute the delta now."); | ||
|
|
||
| // Content does already fit | ||
| // We "just" need to readjust the column widths | ||
| determineNewWidth(userChosenColumnToResize, delta) | ||
| .ifPresent(newWidth -> { | ||
| double currentWidth = userChosenColumnToResize.getWidth(); | ||
| Double deltaToApply = newWidth - currentWidth; | ||
| for (TableColumnBase<?, ?> col : visibleLeafColumns) { | ||
| Double share = expansionShare.get(col); | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Double newSize; | ||
| if (col.equals(prop.getColumn())) { | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // The column resized by the user gets the delta; | ||
| newSize = newWidth; | ||
| } else { | ||
| // The columns not explicitly resized by the user get the negative delta | ||
| newSize = col.getWidth() - share * deltaToApply; | ||
|
There was a problem hiding this comment. I think this doesn't result in a consistent table width. Imagine having two columns, with initial preferred widths of 70/30 and a total table width of 100. Now the user extends the first column by 10, so |
||
| } | ||
| newSize = Math.min(newSize, col.getMaxWidth()); | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| col.setPrefWidth(newSize); | ||
| } | ||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| LOGGER.debug("Window smaller than content"); | ||
|
|
||
| determineNewWidth(userChosenColumnToResize, delta).ifPresent(newWidth -> | ||
| userChosenColumnToResize.setPrefWidth(newWidth)); | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Rearranges the widths of all columns according to their shares (proportional to their preferred widths) | ||
| */ | ||
| private Boolean rearrangeColumns(TableView<?> table) { | ||
| Double tableWidth = getContentWidth(table); | ||
| List<? extends TableColumnBase<?, ?>> visibleLeafColumns = table.getVisibleLeafColumns(); | ||
|
|
||
| Double remainingAvailableWidth = tableWidth - getSumMinWidthOfColumns(table); | ||
|
|
||
| for (TableColumnBase<?, ?> col : visibleLeafColumns) { | ||
| double share = expansionShare.get(col); | ||
| double newSize = col.getMinWidth() + share * remainingAvailableWidth; | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Just to make sure that we are staying under the total table width (due to rounding errors) | ||
| newSize -= 2; | ||
|
|
||
| newSize = Math.min(newSize, col.getMaxWidth()); | ||
|
|
||
| col.setPrefWidth(newSize); | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| return true; | ||
koppor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** | ||
| * Sums up the minimum widths of each column in the table | ||
| */ | ||
| private Double getSumMinWidthOfColumns(TableView<?> table) { | ||
| return table.getColumns().stream().mapToDouble(TableColumnBase::getMinWidth).sum(); | ||
| } | ||
|
|
||
| /** | ||
| * Computes and returns the width required by the content of the table | ||
| */ | ||
| private Double getContentWidth(TableView<?> table) { | ||
| try { | ||
| // TODO: reflective access, should be removed | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the if check should still be there, since otherwise the checkbox in the preferences doesn't do anything, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed the preference option completely - since this was a hidden feature. That should have been a visible toggle button and not hidden in the preferences.
Moreover #967 said that there should be a single behavior without the need of a configuration flat. We discussed it in length back then - and the implementors think, this is still a good thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added a guard to really cover both cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you don't want to give users the option for the infinite-column behavior that you were supporting above? I don't see any harm in still having the checkbox. But since I don't want to argue again for keeping code while you want to remove it, removing is also fine with me ;-)