Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Oct 23, 2021

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Oct 14, 2021

Two things which annoy and confuse users

Refs #8146 #6348

grafik

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 14, 2021
<tooltip>
<Tooltip
text="%When downloading files, or moving linked files to the file directory, prefer the BIB file location rather than the file directory set above"/>
text="%When downloading files, or moving linked files to the file directory, use the BIB file location. This takes precedence over all other configured file directories"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good clarification.

Fullstop missing though

@@ -36,10 +36,10 @@
</tooltip>
</Button>
</HBox>
<CheckBox fx:id="useBibLocationAsPrimary" text="%Search and store files relative to library file location">
<CheckBox fx:id="useBibLocationAsPrimary" text="%Search and store files relative to bib file location">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, "library file" or maybe shorter "library" is more consistent with other usages (e.g. Create new library).
But in either case the tooltip should use the same convention (and not BIB file).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explicitly wanted to use bib file location as we had it back in the past , because otherwise users think they have to enable this for library specific directory which in fact does the opposite

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search and store files relative to library (.bib file) location

@@ -509,7 +509,7 @@ private JabRefPreferences() {
defaults.put(SIZE_X, 1024);
defaults.put(SIZE_Y, 768);
defaults.put(WINDOW_MAXIMISED, Boolean.TRUE);
defaults.put(AUTO_RESIZE_MODE, Boolean.TRUE);
defaults.put(AUTO_RESIZE_MODE, Boolean.FALSE); // Fit table horizontally on the screen
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default is actually correct: auto_resize = automatically resize columns to fit the size of the window. Might it be that at some other point (either UI or in the main table) something went wrong? Or at that at some point it was false, and then this is now written in every users settings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the linked issues. If you start resizing one column, all other column will start to move around back and forth.
if you disable it you have the normal behavior like in Excel or any other table application

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should rename the comment to clarify that this option is disabled by default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the above, this merger could potentially solve / negatively affect:
#5872.

Main merger and reasoning that led to the current default (fit table horizontally on screen = enabled)
#7181
It fixed: #6690 and #967

#6690 does not occur when fit table horizontally on screen is disabled. So that leaves #967.

The effort that was put in to make fit table horizontally on screen work reminds me of the term "sunk cost fallacy" 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i was wrong, i think #7181 was just a major rewrite of fit table horizontally on screen. If i read correctly, fit table horizontally on screen was enabled by default already in 2017 when tobiasdiez introduced his prototype version of the new javaFX Jabref 5.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then I misunderstood the intention of the PR (and yes the comment is somewhat misleading).

I'm the first one that admits that this feature is not perfect and has room for improvement. But in my opinion it is still better than the excel behavior. Indeed, we are not a calculation software for which infinite scrolling makes sense but "only" display our data in form of a table. As a user I find it irritating if part of the data is no longer visible just because I changed the dimension of the window.

Also after #7181 the feature is definitely good enough to be enabled by default, so this would lead to a back-and-forth situation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a matter of fact, i think in future, developers of Jabref will be confronted with similar requests over and over again, as long as new people come to Jabref and have no idea about the option to enable/disable fit horizontally on screen in the preferences.

The reason is that scrolling is intuitive and is able to scale with ever more columns, whereas fit horizontally on screen enabled, at one point will reach a limit where it is just not possible to sustain that option anymore. Indeed, i wish all the stuff i want to look at, would fit on one screen, but it just does not. So at one point (when the screensize < column size; (that includes a change in dimension of the window)) becoming "excel" becomes necessary, not optional. I can't argue the same for fit horizontally on screen enabled.

And this argument holds true, even if there were absolutely 0 issues with fit horizontally on screen enabled.

This is but one reason for fit horizontally on screen to not be enabled by default and i think it is a very compelling reason, but alas, ... i really am curious about how many people enable/disable this option 😄

…idth

* upstream/main:
  Squashed 'buildres/csl/csl-locales/' changes from 495f888637..0cc3885f61
  Refresh example styles
  Squashed 'buildres/csl/csl-styles/' changes from 3b00357..3a6a0a7
  Moved openoffice panel pref to sidepane prefs
  Moved sidepane classes to proper package, reduced scope and merged sidepanemanager with sidepane
  Lazyloading of SidePaneComponents and removed unnecessary BorderPane
  Refactored booleans to set
  Added parse method
  Removed JabRefFrame from constructor arguments in openofficepanel
  Converted SidePanePreferences to new preferences model
…idth

* upstream/main:
  Remove linting exceptions not required any more (and fix blank lines in REAMDE.md)
  Disable markdownlint for docs, because Gitbook does not produce "correct" Markdown
  Disable IacrEprintFetcherTest, because gets blocked
  Fix checkstyle
  Add empty line to have log file rendered properly
  GitBook: [#34] Add link to config of "Auto import feature"
  Adapt test to new behavior
  Add PR reference
  Remove semantic duplicate class Eprint
  Fix ArXiVIdentifier -- dot is required and not arbitrary character
…idth

* upstream/main:
  Bump xmlunit-core from 2.8.2 to 2.8.3
  Bump classgraph from 4.8.126 to 4.8.128
  Allow manual triggering of fetcher tests
  Fix checkstyle
  Move bypass to start of JabRef
@koppor
Copy link
Member

koppor commented Oct 18, 2021

Old:

grafik

Remove:

  • shouldFilesOnOpen
  • shouldOpenBrowserOnCreate

grafik

New:

File directory

(Radio button group - only one can be selected - similar to Autolink files)

  • Main file directory c:\temp
  • Search and store files relative to library file location

Autolink files

(as is)

@koppor
Copy link
Member

koppor commented Oct 18, 2021

This is but one reason for fit horizontally on screen to not be enabled by default and i think it is a very compelling reason, but alas, ... i really am curious about how many people enable/disable this option

Can we add telemetry here?

…idth

* upstream/main:
  Fix some typos (#8157)
  Removed two unused preference options (#8164)
@ilippert
Copy link
Contributor

This is but one reason for fit horizontally on screen to not be enabled by default and i think it is a very compelling reason, but alas, ... i really am curious about how many people enable/disable this option

Can we add telemetry here?

Hi, briefly: even though I somewhat follow development and occasionally adapt preferences, I was not aware of the configuration option in this case. So, I used the default and lived with the "kind of pain". So, the pain kind of experience is not indicated in data that the default setting is/was used.

…idth

* upstream/main:
  Observable preferences E (FilePreferences) (#8165)
  More checkstyle fixes
  Fix checkstyle
  Remove Elsevier test
  Pass-through base URL to allow for relative URL resolution
  Use customBase also at DoiResolution (if enabled)
  Add some code comments
  Revert "Add first try to check for content type"
  Add first try to check for content type
  Add test for paywalled articles
  Change from http to https for the referrer
@Siedlerchr
Copy link
Member Author

grafik

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Oct 22, 2021

Reworked the dialog and added a changelog entry.
DevCall decision was to disable "Fit table horizontally on the screen" as it seems to confuse and annoy users

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions

@@ -28,6 +28,7 @@
public class LinkedFilesTabViewModel implements PreferenceTabViewModel {

private final StringProperty mainFileDirectoryProperty = new SimpleStringProperty("");
private final BooleanProperty useMainFileDirectoryProperty = new SimpleBooleanProperty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that property used at all? If not, I would just delete it and let the radio option handle handle the other property, since that is the only one, that is stored in the preferences

Copy link
Member Author

@Siedlerchr Siedlerchr Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I first had it with the radio button only, but it didn't toggle the radio button correctly and I can't have a bidirectional-binding with not()
Edit// When closing and opening the dialog none of the radio buttons was then selected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i see

Siedlerchr and others added 2 commits October 22, 2021 23:21
…esTab.fxml

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
…esTab.java

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
@Siedlerchr Siedlerchr merged commit 9dbdf13 into main Oct 23, 2021
@Siedlerchr Siedlerchr deleted the disableColumnFitToWidth branch October 23, 2021 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants