Skip to content

Fix for issue 8198, 8133#8229

Merged
Siedlerchr merged 6 commits into
JabRef:mainfrom
sjHong645:fix-for-issue-8192
Nov 14, 2021
Merged

Fix for issue 8198, 8133#8229
Siedlerchr merged 6 commits into
JabRef:mainfrom
sjHong645:fix-for-issue-8192

Conversation

@sjHong645

@sjHong645 sjHong645 commented Nov 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • 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.

I saw the frombooleans method.

'frombooleans' method has 2 parameters.
First parameter needs to be given 'boolean SpecifiedOrder'.
Second parameter needs to be given 'boolean TableOrder'.

Second parameter of previous code was given 'boolean OrigianlOrder'.

So I changed that part.

Fixes #8198

About "JabRefPreferences.java", there is no variable about 'TABLE OrderType'.
So I make it.

Update SaveOrderConfig.java to fix #8133

@sjHong645 sjHong645 changed the title Fix for issue 8192 Fix for issue 8192, 8133 Nov 10, 2021
@sjHong645

sjHong645 commented Nov 10, 2021

Copy link
Copy Markdown
Contributor Author

"Update SaveOrderConfig.java" is about #8133

Kopper's comment about WARN in #8133 gave me a clue.
(WARN org.jabref.model.metadata.SaveOrderConfig - Could not parse sort order: specified - trying to parse the sort criteria)

OrderType enum has three things. SPECIFIED, ORIGINAL, TABLE.
Therefore, correct parameter of 'valueof' function is not 'specified', 'original', 'table' but 'SPECIFIED', 'ORIGINAL', 'TABLE'.

So I attach 'toUpperCase' method to parameter of valueof function.

@koppor koppor requested a review from calixtus November 10, 2021 21:47
@sjHong645 sjHong645 requested a review from koppor November 10, 2021 22:25
@sjHong645

sjHong645 commented Nov 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Sorry @koppor . Request for you is my mistake. :(

@sjHong645 sjHong645 changed the title Fix for issue 8192, 8133 Fix for issue 8198, 8133 Nov 11, 2021
@calixtus

calixtus commented Nov 12, 2021

Copy link
Copy Markdown
Member

Now the preferences variable 'EXPORT_IN_ORIGINAL_ORDER` is ignored.
Please do not create a new preferences variable without need, but fix the wrong logic, otherwise we would also have to introduce preferences migrations.
So what the fromBooleans method should really look like is this:

        public static OrderType fromBooleans(boolean saveInSpecifiedOrder, boolean saveInOriginalOrder) {
            SaveOrderConfig.OrderType orderType = OrderType.TABLE;
            if (saveInSpecifiedOrder) {
                orderType = OrderType.SPECIFIED;
            } else if (saveInOriginalOrder) {
                orderType = OrderType.ORIGINAL;
            }

            return orderType;
        }

Also: the default values need to be changed in the defaults-map in JabRefPreferences, not by altering logic.

toUppercase may sound right, although i thought that valueof is case insensitive...

calixtus
calixtus requested changes Nov 12, 2021
View reviewed changes

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please adress my remarks.

@Siedlerchr Siedlerchr added the status: changes-required Pull requests that are not yet complete label Nov 12, 2021
@sjHong645

sjHong645 commented Nov 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@calixtus

If 'fromBoolean' method is defined as your suggestion,

I think that default value of EXPORT_IN_ORIGINAL_ORDER set 'TRUE' according to your comment.
So I suggest changing as below in JabRefPreferences.java

Because default value of EXPORT_IN_ORIGINAL_ORDER and EXPORT_IN_SPECIFIED_ORDER is all 'FALSE',
Default save order is 'Use current table sort order'

// export order
// defaults.put(EXPORT_IN_ORIGINAL_ORDER, Boolean.FALSE);
defaults.put(EXPORT_IN_ORIGINAL_ORDER, Boolean.TRUE); 
defaults.put(EXPORT_IN_SPECIFIED_ORDER, Boolean.FALSE);

Is it okay?

@sjHong645 sjHong645 requested a review from calixtus November 13, 2021 14:09
@calixtus

calixtus commented Nov 13, 2021

Copy link
Copy Markdown
Member

Yes, that sound right. Thanks.
Go ahead, push a commit and we'll look into it.

@sjHong645

sjHong645 commented Nov 14, 2021

Copy link
Copy Markdown
Contributor Author

@calixtus I did it!

calixtus
calixtus approved these changes Nov 14, 2021
View reviewed changes

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me! Thanks. Looking forward to see more PRs. 😄

@Siedlerchr Siedlerchr merged commit 10a2760 into JabRef:main Nov 14, 2021
@sjHong645

sjHong645 commented Nov 14, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for accepting my PR!!

@sjHong645 sjHong645 deleted the fix-for-issue-8192 branch November 14, 2021 11:45
Siedlerchr added a commit that referenced this pull request Nov 19, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into jabrefonline
6aad7f2 
* upstream/main: (181 commits)
  Add of ADRs 22 and 23 (#8256)
  [Bot] Update CSL styles (#8245)
  Replace styfle/cancel-workflow-action@0.9.1 by GitHub's "concurrency" feature (#8243)
  Bump gittools/actions from 0.9.10 to 0.9.11 (#8248)
  Bump commons-cli from 1.4 to 1.5.0 (#8250)
  Bump byte-buddy-parent from 1.12.0 to 1.12.1 (#8249)
  Bump antlr4 from 4.9.2 to 4.9.3 (#8251)
  Bump archunit-junit5-api from 0.21.0 to 0.22.0 (#8252)
  Fix search: NOT binds more than AND (#8241)
  New Crowdin updates (#8240)
  Make PdfGrobiImporterTest as FetcherTest
  Oobranch g : add actions (#7792)
  Fix mixed CRLF / CR (#8238)
  Fix "Library has changed externally" with CRLF markers (#8239)
  Fix for issue 8198, 8133 (#8229)
  Added more unit tests in AuthorTest (#8214)
  Add confirmation dialog for empty entries in JabRef (#8218)
  Fix entry editor column visibility (#8232)
  Use regexp to remove non-ASCII characters from DOI and inform user when data for valid DOI does not exist #8127 (#8228)
  Fix exception for search flags (#8237)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@calixtus calixtus calixtus approved these changes

@koppor koppor Awaiting requested review from koppor

Assignees

No one assigned

Labels

status: changes-required Pull requests that are not yet complete

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Save sort order switching between "Keep original order" and "Use current table sort order" Default save order should be "Keep original order"

4 participants

@sjHong645 @calixtus @Siedlerchr @koppor