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.

Sign up for GitHub

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

minor refactor to JabRefDialogService #11767

Merged
merged 5 commits into from
Sep 18, 2024
Merged

minor refactor to JabRefDialogService #11767

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c6d62bc
minor refactor to JabRefDialogService
leaf-soba Sep 16, 2024
fd3b7f0
Update JabRefDialogService
leaf-soba Sep 17, 2024
53d046c
fix missing Localization.lang
leaf-soba Sep 17, 2024
15ec6de
fix missing string
leaf-soba Sep 17, 2024
a9fcc67
fix Localization.lang issue
leaf-soba Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 32 additions & 31 deletions src/main/java/org/jabref/gui/JabRefDialogService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@
import java.nio.file.FileSystems;
import java.nio.file.Path;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.function.Consumer;
import java.util.stream.Collectors;

import javafx.concurrent.Task;
import javafx.geometry.Pos;
Expand Down Expand Up @@ -99,9 +97,9 @@ private FXDialog createDialog(AlertType type, String title, String content) {
return alert;
}

private FXDialog createDialogWithOptOut(AlertType type, String title, String content,
private FXDialog createDialogWithOptOut(String title, String content,
String optOutMessage, Consumer<Boolean> optOutAction) {
FXDialog alert = new FXDialog(type, title, true);
FXDialog alert = new FXDialog(AlertType.CONFIRMATION, title, true);
// Need to force the alert to layout in order to grab the graphic as we are replacing the dialog pane with a custom pane
alert.getDialogPane().applyCss();
Node graphic = alert.getDialogPane().getGraphic();
Expand Down Expand Up @@ -135,7 +133,7 @@ public static String shortenDialogMessage(String dialogMessage) {
if (dialogMessage.length() < JabRefDialogService.DIALOG_SIZE_LIMIT) {
return dialogMessage.trim();
}
return (dialogMessage.substring(0, Math.min(dialogMessage.length(), JabRefDialogService.DIALOG_SIZE_LIMIT)) + "...").trim();
return (dialogMessage.substring(0, JabRefDialogService.DIALOG_SIZE_LIMIT) + "...").trim();
}

private <T> ChoiceDialog<T> createChoiceDialog(String title, String content, String okButtonLabel, T defaultChoice, Collection<T> choices) {
Expand Down Expand Up @@ -226,16 +224,7 @@ public void showErrorDialogAndWait(FetcherException fetcherException) {
String localizedMessage = fetcherException.getLocalizedMessage();
Optional<SimpleHttpResponse> httpResponse = fetcherException.getHttpResponse();
if (httpResponse.isPresent()) {
int statusCode = httpResponse.get().statusCode();
if (statusCode == 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);
} else if (statusCode == 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);
} else if (statusCode == 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);
} 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@leaf-soba leaf-soba Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@leaf-soba leaf-soba Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Discussion follow-up at #11784

} else if (fetcherException instanceof FetcherClientException) {
this.showErrorDialogAndWait(failedTitle, Localization.lang("Something is wrong on JabRef side. Please check the URL and try again.") + "\n\n" + localizedMessage);
} else if (fetcherException instanceof FetcherServerException) {
Expand All @@ -246,6 +235,19 @@ public void showErrorDialogAndWait(FetcherException fetcherException) {
}
}

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.";
Copy link
Member

Choose a reason for hiding this comment

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

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

case 403 ->
"Access denied. You do not have permission to access this resource. Please contact the administrator for assistance or try a different action.";
case 404 ->
"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.";
default ->
"Something is wrong on JabRef side. Please check the URL and try again.";
};
}

@Override
public void showErrorDialogAndWait(String title, String content, Throwable exception) {
ExceptionDialog exceptionDialog = new ExceptionDialog(exception);
Expand Down Expand Up @@ -288,7 +290,7 @@ public boolean showConfirmationDialogAndWait(String title, String content,
@Override
public boolean showConfirmationDialogWithOptOutAndWait(String title, String content,
String optOutMessage, Consumer<Boolean> optOutAction) {
FXDialog alert = createDialogWithOptOut(AlertType.CONFIRMATION, title, content, optOutMessage, optOutAction);
FXDialog alert = createDialogWithOptOut(title, content, optOutMessage, optOutAction);
alert.getButtonTypes().setAll(ButtonType.YES, ButtonType.NO);
return alert.showAndWait().filter(buttonType -> buttonType == ButtonType.YES).isPresent();
}
Expand All @@ -297,7 +299,7 @@ public boolean showConfirmationDialogWithOptOutAndWait(String title, String cont
public boolean showConfirmationDialogWithOptOutAndWait(String title, String content,
String okButtonLabel, String cancelButtonLabel,
String optOutMessage, Consumer<Boolean> optOutAction) {
FXDialog alert = createDialogWithOptOut(AlertType.CONFIRMATION, title, content, optOutMessage, optOutAction);
FXDialog alert = createDialogWithOptOut(title, content, optOutMessage, optOutAction);
ButtonType okButtonType = new ButtonType(okButtonLabel, ButtonBar.ButtonData.YES);
ButtonType cancelButtonType = new ButtonType(cancelButtonLabel, ButtonBar.ButtonData.NO);
alert.getButtonTypes().setAll(okButtonType, cancelButtonType);
Expand Down Expand Up @@ -388,7 +390,7 @@ public <V> void showProgressDialogAndWait(String title, String content, Task<V>
}

@Override
public <V> Optional<ButtonType> showBackgroundProgressDialogAndWait(String title, String content, StateManager stateManager) {
public Optional<ButtonType> showBackgroundProgressDialogAndWait(String title, String content, StateManager stateManager) {
TaskProgressView<Task<?>> taskProgressView = new TaskProgressView<>();
EasyBind.bindContent(taskProgressView.getTasks(), stateManager.getRunningBackgroundTasks());
taskProgressView.setRetainTasks(false);
Expand Down Expand Up @@ -424,25 +426,24 @@ public void notify(String message) {
// The event log is not that user friendly (different purpose).
LOGGER.info(message);

UiTaskExecutor.runInJavaFXThread(() -> {
UiTaskExecutor.runInJavaFXThread(() ->
Notifications.create()
.text(message)
.position(Pos.BOTTOM_CENTER)
.hideAfter(TOAST_MESSAGE_DISPLAY_TIME)
.owner(mainWindow)
.threshold(5,
Notifications.create()
.title(Localization.lang("Last notification"))
.text(
"(" + Localization.lang("Check the event log to see all notifications") + ")"
+ "\n\n" + message)
.onAction(e -> {
ErrorConsoleAction ec = new ErrorConsoleAction();
ec.execute();
}))
Notifications.create()
.title(Localization.lang("Last notification"))
.text(
"(" + Localization.lang("Check the event log to see all notifications") + ")"
+ "\n\n" + message)
.onAction(e -> {
ErrorConsoleAction ec = new ErrorConsoleAction();
ec.execute();
}))
.hideCloseButton()
.show();
});
.show());
}

@Override
Expand Down Expand Up @@ -472,7 +473,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() : List.of();
}

private DirectoryChooser getConfiguredDirectoryChooser(DirectoryDialogConfiguration directoryDialogConfiguration) {
Expand Down
Loading