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

Fix for issue 8198, 8133 #8229

Merged
merged 6 commits into from
Nov 14, 2021
Merged

Conversation

sjHong645
Copy link
Contributor

@sjHong645 sjHong645 commented Nov 9, 2021

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

sjHong645 commented Nov 10, 2021

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
Copy link
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...

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.

please adress my remarks.

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

sjHong645 commented Nov 13, 2021

@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
Copy link
Member

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

@sjHong645
Copy link
Contributor Author

@calixtus I did it!

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.

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

@Siedlerchr Siedlerchr merged commit 10a2760 into JabRef:main Nov 14, 2021
@sjHong645
Copy link
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
* 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
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
4 participants