Implement better scaling of main table showing entries #7181

koppor · 2020-12-12T22:36:47Z

Fixes #967 so that resizing works "naturally".

~~The only missing thing is following:~~

~~If there is additional space left the remaining columns could be broadend to use this space.~~

~~This is future work. Proposal: Additional button doing this "magic". Thus, the user has control when other than the current column should be resized.~~

~~With the current implementation, JabRef is consistent with LibreOffice Calc's behavior (and other table calculation tools).~~

~~Still open: We could not reproduce #6690 because of #7180~~

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

tobiasdiez · 2020-12-13T13:25:29Z

I think what you implemented was already possible before by unchecking "Entry table > Fit table horizontally on screen" in the preferences (except if I misunderstood what you were trying to do).

(Also I think one shouldn't take calculation programs as inspiration since they usually have the concept of infinitely many columns.)

src/main/java/org/jabref/gui/maintable/MainTableColumnFactory.java

koppor · 2020-12-14T19:02:11Z

I think what you implemented was already possible before by unchecking "Entry table > Fit table horizontally on screen" in the preferences (except if I misunderstood what you were trying to do).

Not quite:

We implemented minimum width:

(shown in the dev call)

The "Fit table horitzontally on screen" is broken:

https://www.loom.com/share/70a2a976a5164619a03ceaa150ef2cdf

Columns can be shrunk until they are not visible anymore
Columns on the right "jump" while moving.

We are still thinking how to proceed. In my view, a "magic alignment button" setting an "optimal" cell width and then the user can adjust the widths manually is a least WTF than the current system.

For sure, we could work on something more "intelligent". I am not sure about the user experience if we are the only tool doing that way.

(Also I think one shouldn't take calculation programs as inspiration since they usually have the concept of infinitely many columns.)

Nevertheless, there JabRef typically has 15 columns.

The only tool I know with more than 10 columns is a spreadsheet application. Regarding the "Ewartungskonformität" (DIN 66234 Teil 8), this is the only thing raising expectations. Not sure whether other users also know spread sheet applications. However, I would guess so.

koppor · 2020-12-14T19:30:14Z

Windows Explorer:

Double click should set the optimum width:

- Remove preferences - Remove MainTablePreferences (and migrate left-overs into ColumnPreferences) - Fix name of "Show dedicated file columns" (instead of "Show extra columns") Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor · 2020-12-14T21:53:53Z

DevCall decision:

Merge and work on column resizing afterwards. Probably with the magic button as in Windows Explorer.

tobiasdiez · 2020-12-15T13:25:04Z

Well, the no-automatic-column-resizing strategy is the default one of JavaFX and can be enabled by unchecking the above checkbox. So no code changes required for this.

While I agree that the smart resizing policy has some flaws, it was working relatively well (for me and many users).

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor · 2020-12-17T22:47:51Z

Is this really the first time, I want to delete code and @tobiasdiez wants to keep it? 😇

@DominikVoigt and I worked hard to keep the smart resize. Here it is, freshly implemented in a (hopefully) smart way. Tested locally. Enjoyed it.

tobiasdiez

Thanks a lot for the update. Looks way better now. (And yes I like to keep useful code ;-) ).

I think I've found a few cases where the new column calculation doesn't work as expected. It would be very useful to have unit tests covering a few different scenarios. This would also give more confidence in the changes, also in the future (because I'm pretty sure we will miss some edge cases - the resize thing is really complicated sadly).
Here is a blueprint for a test: https://github.com/openjdk/jfx/blob/a128952b834fc57934c088aff1e024706dab2c5a/modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java#L1948-L1978

tobiasdiez · 2020-12-18T09:42:23Z

src/main/java/org/jabref/gui/maintable/MainTable.java

-        if (mainTablePreferences.getResizeColumnsToFit()) {
-            this.setColumnResizePolicy(new SmartConstrainedResizePolicy());
-        }
+        this.setColumnResizePolicy(new SmartConstrainedResizePolicy());


I think the if check should still be there, since otherwise the checkbox in the preferences doesn't do anything, right?

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.

We added a guard to really cover both cases

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 ;-)

src/main/java/org/jabref/gui/maintable/MainTableColumnFactory.java

src/main/java/org/jabref/gui/maintable/SmartConstrainedResizePolicy.java

tobiasdiez · 2020-12-18T10:42:10Z

src/main/java/org/jabref/gui/maintable/SmartConstrainedResizePolicy.java

+                                newSize = newWidth;
+                            } else {
+                                // The columns not explicitly resized by the user get the negative delta
+                                newSize = col.getWidth() - share * deltaToApply;


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 deltaToApply = 10. The first column then indeed has size 80 (as expected) but the second one has newSize = 30 - 0.3 * 10 = 27 although it should be 20.

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor · 2020-12-21T21:14:07Z

We had some WTF moments with minwidth 10, 29. We tried with 80:

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor · 2021-01-18T19:27:39Z

We hit https://stackoverflow.com/a/47860355/873282. @koppor @DominikVoigt : Look at #7209. Should be easy

Expected behavior: double click: "optimal" column width. Iterate on content an choose the maximum content length as prefWidth.

koppor · 2021-02-01T20:34:20Z

We proceed as follows:

@calixtus resolves merge conflicts
Manual test of new system
Merge
Tests will come as follow up (otherwise, this won't happen until August 2021)
The issue with double click on a column leading to a wrong resize will be fixed later Implement better scaling of main table showing entries #7181 (comment)
Right-click on table column headers will enable "fixed" columns which should improve the UX even more
Right-click on table column will allow to enable standard resize policy again (which has the effect that enlarging a column does not shrink others). This re-introduces the standard-behavior of windows explorer
Right-lock on table column will offer "reaarange all columns" (because when changing from windows explorer policy to smart resize policy could lead to bad column widths)

tobiasdiez

Ok, it would still be good if you could at least add some basic tests.

Hopefuly this shouldn't be too hard. Create a table, with a fixed width. Then call the reize policy and verify the result.

tobiasdiez · 2021-02-02T20:47:33Z

src/main/resources/log4j2.xml

        </Console>
        <GuiAppender name="GuiLogger"/>
        <OurApplicationInsightsAppender name="applicationInsightsAppender"/>
+        <File name="FILE" fileName="jabref.log">


These changes here probably need to be reverted, right?

tobiasdiez · 2021-02-02T20:50:16Z

src/main/java/org/jabref/gui/maintable/SmartConstrainedResizePolicy.java

+    /**
+     * SIDE EFFECT: Stores computed desired width if not present in HashMap. This leads to a constant desired width.
+     */
+    private Double getDesiredColumnWidth(TableColumn column) {


then rename to computeDesiredWidth

# Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.java # src/main/java/org/jabref/gui/preferences/table/TableTabViewModel.java

calixtus · 2021-02-06T11:44:07Z

After a little bit testing, I made those observations:

Minimal column width is too large: my year column is three times to big at minimum size.
Contraintuitive behaviour: if I change the width of a column, the column border is not on the same x position as my mouse cursor is. The mouse cursor seems to move in-proportionally more than the column border.
After closing an reopening JabRef stored column widths are not reloaded

calixtus · 2021-02-06T11:56:51Z

I fixed the merge conflicts as promised, but I believe, there are probably more cleanups to do...

koppor · 2021-02-07T21:44:07Z

After a little bit testing, I made those observations:
* Minimal column width is too large: my year column is three times to big at minimum size.

Depends on the other columns and the other columns. We decided not to add a fixed width to the year column. We hope for a context menu to have the year column fixed. IMHO 99% the users will use the fixed width, only some non-mainstream research will use more than 4 digits for a year

* Contraintuitive behaviour: if I change the width of a column, the column border is not on the same x position as my mouse cursor is. The mouse cursor seems to move in-proportionally more than the column border.

We know that. At first, it also seemed to be counter-intuitive. Later, we liked it somehow (and hoped for some feedback). -- The only possibility is to change the behavior that only columns right of the current column are resized. Maybe, this is the way to go.

* After closing an reopening JabRef stored column widths are not reloaded

Works as designed. See

jabref/src/main/java/org/jabref/gui/maintable/SmartConstrainedResizePolicy.java

Line 66 in 286e7f9

    
            * Decision to take: a) Use last setting or b) rerender with desired widths. We opt for b), because we assume that user-configuration of desired widths is easy. This leads to I3 being inconsistent to RWS2 (there is no previous width in I3)

for a reasoning.

I repeat it here

Initial table rendering

Decision to take: a) Use last setting or b) rerender with desired widths. We opt for b), because we assume that user-configuration of desired widths is easy. This leads to I3 being inconsistent to RWS2 (there is no previous width in I3).

tobiasdiez · 2021-02-07T22:26:32Z

Restoring the last column widths works currently and is not something that should be controlled by the resize policy.
My guess is that
https://github.com/JabRef/jabref/pull/7181/files#diff-535037ff480247bb2ecacf90ae5281974308e819eb2c3eed6b5cb15931a077caR122-R125
is overwriting

jabref/src/main/java/org/jabref/gui/maintable/columns/MainTableColumn.java

Lines 17 to 21 in e66f5be

    
           BindingsHelper.bindBidirectional( 
        
                   this.widthProperty(), 
        
                   model.widthProperty(), 
        
                   value -> this.setPrefWidth(model.widthProperty().getValue()), 
        
                   value -> model.widthProperty().setValue(this.getWidth()));

calixtus · 2021-02-15T19:44:50Z

Dev-Call decision: restore last state

Needed to add "preferences.getColumnPreferences().isDedicatedFileColumnsEnabled()))" to PersitenceVisualStateTable.java # Conflicts: # docs/adr/0016-mutable-preferences-objects.md # src/main/java/org/jabref/gui/maintable/PersistenceVisualStateTable.java # src/main/java/org/jabref/gui/maintable/SmartConstrainedResizePolicy.java # src/main/resources/l10n/JabRef_en.properties

Siedlerchr · 2022-09-13T18:44:51Z

Related openjdk/jfx#897

https://bugs.openjdk.org/browse/JDK-8293119

koppor · 2023-05-07T13:40:05Z

I didn't have the time to finish it. Moving to koppor to reduce the number of PRs here. koppor#644

Implement better scaling of main table showing entries

a1a5ccc

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor force-pushed the fix-967 branch from 2fdb153 to a1a5ccc Compare December 12, 2020 22:44

tobiasdiez added the status: changes required Pull requests that are not yet complete label Dec 13, 2020

Siedlerchr reviewed Dec 13, 2020

View reviewed changes

src/main/java/org/jabref/gui/maintable/MainTableColumnFactory.java Outdated Show resolved Hide resolved

koppor and others added 2 commits December 14, 2020 20:32

Fix number

e91bb8d

koppor mentioned this pull request Dec 14, 2020

Activation of "Show dedicated file columns" should add columns of existing files koppor/jabref#477

Open

Fix checkstyle

1acd12e

koppor and others added 2 commits December 17, 2020 20:27

Merge branch 'master' into fix-967

c8caa13

Add smart resize if content fits into table width

f5e2d9f

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor removed the status: changes required Pull requests that are not yet complete label Dec 17, 2020

tobiasdiez requested changes Dec 18, 2020

View reviewed changes

tobiasdiez added the status: changes required Pull requests that are not yet complete label Dec 18, 2020

koppor and others added 10 commits December 19, 2020 00:17

Address some comments

bcea66c

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

WIP

6da49b2

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Speedup processResources

dfac705

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Merge branch 'speedup-process-resources' into fix-967

057ae9f

Try path without ./ at the beginning

8834fb6

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Output java error on console, too

5b6f0d8

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Use LOGGER instead of System.err

fd80c30

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Merge branch 'fix-output-with-wrong-jdk-version' into fix-967

c64b5e3

Merge branch 'speedup-process-resources' into fix-967

9f56fd5

Fix scroll bar still present

d15e90f

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

koppor and others added 6 commits January 6, 2021 16:55

WIP: Reimplement logic according to comments

496e505

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Merge remote-tracking branch 'origin/master' into fix-967

466731f

WIP

caf1e23

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Merge remote-tracking branch 'origin/master' into fix-967

63af8fd

Resizing of colomms works

274e109

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Use EPSILON_MARGIN also for "content has fit into table before"

910c592

Co-authored-by: Dominik Voigt <dominik.ingo.voigt@gmail.com>

Merge branch 'master' into fix-967

513c68c

tobiasdiez reviewed Feb 2, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-967

286e7f9

# Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.java # src/main/java/org/jabref/gui/preferences/table/TableTabViewModel.java

koppor mentioned this pull request Feb 7, 2021

Fix typos #7425

Merged

5 tasks

koppor mentioned this pull request Sep 2, 2021

Global Search UI issues #8054

Closed

4 tasks

ThiloteE mentioned this pull request Oct 14, 2021

Disable fit to width by default and rename linked file settings #8148

Merged

5 tasks

koppor self-assigned this Nov 14, 2021

ThiloteE added the maintable label Apr 20, 2022

koppor mentioned this pull request Sep 26, 2022

Resizing of columns when changing displays #5872

Open

koppor mentioned this pull request Nov 2, 2022

Adjust the coumn width is really painful #8146

Closed

koppor mentioned this pull request May 7, 2023

Implement better scaling of main table showing entries koppor/jabref#644

Draft

koppor closed this May 7, 2023

koppor deleted the fix-967 branch October 24, 2023 23:23

koppor mentioned this pull request Oct 24, 2023

Column width cannot be changed #10516

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement better scaling of main table showing entries #7181

Implement better scaling of main table showing entries #7181

koppor commented Dec 12, 2020 •

edited

Loading

tobiasdiez commented Dec 13, 2020

koppor commented Dec 14, 2020 •

edited

Loading

koppor commented Dec 14, 2020

koppor commented Dec 14, 2020

tobiasdiez commented Dec 15, 2020

koppor commented Dec 17, 2020

tobiasdiez left a comment •

edited

Loading

tobiasdiez Dec 18, 2020

koppor Dec 18, 2020 •

edited

Loading

koppor Dec 18, 2020

tobiasdiez Dec 19, 2020

tobiasdiez Dec 18, 2020

koppor commented Dec 21, 2020

koppor commented Jan 18, 2021

koppor commented Feb 1, 2021

tobiasdiez left a comment

tobiasdiez Feb 2, 2021

tobiasdiez Feb 2, 2021

calixtus commented Feb 6, 2021 •

edited

Loading

calixtus commented Feb 6, 2021

koppor commented Feb 7, 2021

tobiasdiez commented Feb 7, 2021

calixtus commented Feb 15, 2021

Siedlerchr commented Sep 13, 2022 •

edited by calixtus

Loading

koppor commented May 7, 2023

Implement better scaling of main table showing entries #7181

Implement better scaling of main table showing entries #7181

Conversation

koppor commented Dec 12, 2020 • edited Loading

tobiasdiez commented Dec 13, 2020

koppor commented Dec 14, 2020 • edited Loading

koppor commented Dec 14, 2020

koppor commented Dec 14, 2020

tobiasdiez commented Dec 15, 2020

koppor commented Dec 17, 2020

tobiasdiez left a comment • edited Loading

Choose a reason for hiding this comment

tobiasdiez Dec 18, 2020

Choose a reason for hiding this comment

koppor Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

koppor Dec 18, 2020

Choose a reason for hiding this comment

tobiasdiez Dec 19, 2020

Choose a reason for hiding this comment

tobiasdiez Dec 18, 2020

Choose a reason for hiding this comment

koppor commented Dec 21, 2020

koppor commented Jan 18, 2021

koppor commented Feb 1, 2021

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Feb 2, 2021

Choose a reason for hiding this comment

tobiasdiez Feb 2, 2021

Choose a reason for hiding this comment

calixtus commented Feb 6, 2021 • edited Loading

calixtus commented Feb 6, 2021

koppor commented Feb 7, 2021

tobiasdiez commented Feb 7, 2021

calixtus commented Feb 15, 2021

Siedlerchr commented Sep 13, 2022 • edited by calixtus Loading

koppor commented May 7, 2023

koppor commented Dec 12, 2020 •

edited

Loading

koppor commented Dec 14, 2020 •

edited

Loading

tobiasdiez left a comment •

edited

Loading

koppor Dec 18, 2020 •

edited

Loading

calixtus commented Feb 6, 2021 •

edited

Loading

Siedlerchr commented Sep 13, 2022 •

edited by calixtus

Loading