minor refactor to JabRefDialogService #11767

leaf-soba · 2024-09-16T12:32:39Z

replace multiple if-else to switch case
remove unused generics <V>
replace .collect(Collectors.toList()) to .toList()
remove unnecessary braces

I'm no sure do we really need this kind of small refactor, please review and let me know if this change aligns with our goals.

Mandatory checks

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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
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.

1. replace multiple if-else to switch case 2. remove unused generics `<V>` 3. replace `.collect(Collectors.toList())` to `.toList()` 4. remove unnecessary braces

koppor · 2024-09-16T13:40:25Z

src/main/java/org/jabref/gui/JabRefDialogService.java

+            switch (statusCode) {
+                case 401 ->
+                    this.showInformationDialogAndWait(failedTitle, Localization.lang("Access denied. You are not authorized to access this resource. Please check your credentials and try again. If you believe you should have access, please contact the administrator for assistance.") + "\n\n" + localizedMessage);
+                case 403 ->
+                    this.showInformationDialogAndWait(failedTitle, Localization.lang("Access denied. You do not have permission to access this resource. Please contact the administrator for assistance or try a different action.") + "\n\n" + localizedMessage);
+                case 404 ->
+                    this.showInformationDialogAndWait(failedTitle, Localization.lang("The requested resource could not be found. It seems that the file you are trying to download is not available or has been moved. Please verify the URL and try again. If you believe this is an error, please contact the administrator for further assistance.") + "\n\n" + localizedMessage);
+                default ->
+                    this.showErrorDialogAndWait(failedTitle, Localization.lang("Something is wrong on JabRef side. Please check the URL and try again.") + "\n\n" + localizedMessage);
            }


This can be refactored even more?

content = switch (statusCode) { 401 -> Localization.lang".."); this.showInformationDialogAndWait(failedTitle, content);

I would just not care about info and error. I think "info" is OK even in the default case.

koppor · 2024-09-16T13:41:01Z

src/main/java/org/jabref/gui/JabRefDialogService.java

@@ -472,7 +470,7 @@ public Optional<Path> showDirectorySelectionDialog(DirectoryDialogConfiguration
    public List<Path> showFileOpenDialogAndGetMultipleFiles(FileDialogConfiguration fileDialogConfiguration) {
        FileChooser chooser = getConfiguredFileChooser(fileDialogConfiguration);
        List<File> files = chooser.showOpenMultipleDialog(mainWindow);
-        return files != null ? files.stream().map(File::toPath).collect(Collectors.toList()) : Collections.emptyList();
+        return files != null ? files.stream().map(File::toPath).toList() : Collections.emptyList();


Suggested change

return files != null ? files.stream().map(File::toPath).toList() : Collections.emptyList();

return files != null ? files.stream().map(File::toPath).toList() : List.of();

koppor · 2024-09-16T13:42:12Z

src/main/java/org/jabref/gui/JabRefDialogService.java

+                                                        .hideAfter(TOAST_MESSAGE_DISPLAY_TIME)
+                                                        .owner(mainWindow)
+                                                        .threshold(5,
+                             Notifications.create()


This new indent is a bit strange - maybe keep the old indent - on line 428 it also "felt" better, because it is indented 4 spaces, not 30 or so ^^

Siedlerchr · 2024-09-16T14:37:59Z

Thanks for your contribution, however due to our limited time for reviews (we are all working at JabRef in our spare time), we prefer such code quality only refactorings more in the context of bugs/feature implementations

calixtus · 2024-09-16T16:37:53Z

Thanks for your interest in JabRef.
As @Siedlerchr said, we as the maintainers try to focus on Bugfixes and features und prefer small refactorings as part of bigger feature PRs or as part of our long term development roadmap. we would like to encourage you to pick a good first issue or another issue to fix or even create a new issue with an new feature idea we could discuss with you.
Since the work is already done (please integrate @koppor s suggestions), we can probably integrate them, when it's done.

koppor · 2024-09-16T18:45:44Z

I'm no sure do we really need this kind of small refactor, please review and let me know if this change aligns with our goals.

For me, it is currently OK, because the batch sizes are small enough to cope with it in some spare minutes 😅.

leaf-soba · 2024-09-17T01:22:01Z

Thanks for your interest in JabRef. As @Siedlerchr said, we as the maintainers try to focus on Bugfixes and features und prefer small refactorings as part of bigger feature PRs or as part of our long term development roadmap. we would like to encourage you to pick a good first issue or another issue to fix or even create a new issue with an new feature idea we could discuss with you. Since the work is already done (please integrate @koppor s suggestions), we can probably integrate them, when it's done.

OK, I'll try my best to solve some bigger issue from now on. Would you and @koppor mind review the comments in #10371 (comment), I'm not sure where to start solve this issue so I break down into small task, but I didn't get reply in two weeks, I think I may make a mistake, such as not @ anyone, so there wasn't any notification to others.

1. remove `type` parameter in createDialogWithOptOut, since it always be `AlertType.CONFIRMATION` 2. remove `Math.min` in `shortenDialogMessage`, since it after a if compare, it means the dialogMessage.length() is always not less than JabRefDialogService.DIALOG_SIZE_LIMIT 3. refactor the switch case more. 4. fix the indent issue.

koppor · 2024-09-17T07:55:32Z

src/main/java/org/jabref/gui/JabRefDialogService.java

+    private String getContentByCode(int statusCode) {
+        return switch (statusCode) {
+            case 401 ->
+                "Access denied. You are not authorized to access this resource. Please check your credentials and try again. If you believe you should have access, please contact the administrator for assistance.";


Please wrap in Localization.lang. See https://devdocs.jabref.org/code-howtos/localization.html#localization-in-java-code for reasons.

fix missing Localization.lang

add missing newline character and localizedMessage

koppor · 2024-09-17T12:44:01Z

src/main/java/org/jabref/gui/JabRefDialogService.java

-            } else {
-                this.showErrorDialogAndWait(failedTitle, Localization.lang("Something is wrong on JabRef side. Please check the URL and try again.") + "\n\n" + localizedMessage);
-            }
+            this.showInformationDialogAndWait(failedTitle, Localization.lang(getContentByCode(httpResponse.get().statusCode())) + "\n\n" + localizedMessage);


No, that does not work.

Localization.lang has to be parameterized with a literal (!) string. Otherwise, our tooling does nt work. Please see https://devdocs.jabref.org/code-howtos/localization.html

I found this TODO in org.jabref.logic.l10n.LocalizationConsistencyTest:

TODO: Localization.lang(var1 + "test" + var2) not covered

In my code is a var1 + "test" + var2 case, so it failed. maybe I should finish this TODO to pass the unit test?

sorry I made a mistake, my code is not var1 + "test" + var2 case, it just a var case which is illegal in LocalizationConsistencyTest

Finishing that ToDo would require several hours of work and design planning. Localization requires full sentences to be translated as in different languages word order and grammar changes d pending on what that var may be. So please just do as @koppor said, fix your strings.

Finishing that ToDo would require several hours of work and design planning. Localization requires full sentences to be translated as in different languages word order and grammar changes d pending on what that var may be. So please just do as @koppor said, fix your strings.

I did as @koppor said, have fixed my strings before I commented. I just want to make this unit test better by finishing ToDo. And I think it may not require several hours work, such as this simple way:

assert code in LocalizationConsistencyTest now:

assertTrue(e.getKey().startsWith("\"") || e.getKey().endsWith("\""), "Illegal localization parameter found. Must include a String with potential concatenation or replacement parameters. Illegal parameter: Localization.lang(" + e.getKey());

a little fix, add StringUtils.countMatches(e.getKey(), "\"") >= 2 to cover Localization.lang(var1 + "test" + var2) case:

assertTrue(e.getKey().startsWith("\"") || e.getKey().endsWith("\"") || StringUtils.countMatches(e.getKey(), "\"") >= 2, "Illegal localization parameter found. Must include a String with potential concatenation or replacement parameters. Illegal parameter: Localization.lang(" + e.getKey());

Discussion follow-up at #11784

need to use plain string not a method result or a var to pass the unit test.

…xArxivHtmlImport * 'fixArxivHtmlImport' of github.com:JabRef/jabref: Fix focus for keywords and crossref fields (#11792) Fix ai chat not on fx thread (#11796) [AI] Add more uses statements (#11788) Update djl api dependency (#11787) Improve pdf content parser for DOIs (#11782) minor refactor to JabRefDialogService (#11767) Add more OS-dependent context to panel freeze dev documentation. (#11781)

minor refactor to JabRefDialogService

c6d62bc

1. replace multiple if-else to switch case 2. remove unused generics `<V>` 3. replace `.collect(Collectors.toList())` to `.toList()` 4. remove unnecessary braces

koppor requested changes Sep 16, 2024

View reviewed changes

koppor added the type: code-quality Issues related to code or architecture decisions label Sep 16, 2024

leaf-soba requested a review from koppor September 17, 2024 01:49

koppor approved these changes Sep 17, 2024

View reviewed changes

koppor requested changes Sep 17, 2024

View reviewed changes

leaf-soba added 2 commits September 17, 2024 16:17

fix missing Localization.lang

53d046c

fix missing Localization.lang

fix missing string

15ec6de

add missing newline character and localizedMessage

koppor requested changes Sep 17, 2024

View reviewed changes

fix Localization.lang issue

a9fcc67

need to use plain string not a method result or a var to pass the unit test.

leaf-soba requested a review from koppor September 17, 2024 23:29

koppor approved these changes Sep 18, 2024

View reviewed changes

koppor added this pull request to the merge queue Sep 18, 2024

Merged via the queue into JabRef:main with commit 88fc846 Sep 18, 2024
23 checks passed

leaf-soba deleted the refactor-JabRefDialogService branch September 26, 2024 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minor refactor to JabRefDialogService #11767

minor refactor to JabRefDialogService #11767

leaf-soba commented Sep 16, 2024

koppor Sep 16, 2024

koppor Sep 16, 2024

koppor Sep 16, 2024

Siedlerchr commented Sep 16, 2024

calixtus commented Sep 16, 2024 •

edited

Loading

koppor commented Sep 16, 2024

leaf-soba commented Sep 17, 2024

koppor Sep 17, 2024

koppor Sep 17, 2024

leaf-soba Sep 17, 2024 •

edited

Loading

calixtus Sep 18, 2024

leaf-soba Sep 18, 2024 •

edited

Loading

koppor Sep 18, 2024

	return files != null ? files.stream().map(File::toPath).toList() : Collections.emptyList();
	return files != null ? files.stream().map(File::toPath).toList() : List.of();

minor refactor to JabRefDialogService #11767

minor refactor to JabRefDialogService #11767

Conversation

leaf-soba commented Sep 16, 2024

Mandatory checks

koppor Sep 16, 2024

Choose a reason for hiding this comment

koppor Sep 16, 2024

Choose a reason for hiding this comment

koppor Sep 16, 2024

Choose a reason for hiding this comment

Siedlerchr commented Sep 16, 2024

calixtus commented Sep 16, 2024 • edited Loading

koppor commented Sep 16, 2024

leaf-soba commented Sep 17, 2024

koppor Sep 17, 2024

Choose a reason for hiding this comment

koppor Sep 17, 2024

Choose a reason for hiding this comment

leaf-soba Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

calixtus Sep 18, 2024

Choose a reason for hiding this comment

leaf-soba Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

koppor Sep 18, 2024

Choose a reason for hiding this comment

calixtus commented Sep 16, 2024 •

edited

Loading

leaf-soba Sep 17, 2024 •

edited

Loading

leaf-soba Sep 18, 2024 •

edited

Loading