Added option in preferences to use custom URL base for DOI generation#7480
Conversation
calixtus
left a comment
There was a problem hiding this comment.
Hi @BJaroszkowski , thanks for your contribution. Codewise the changes already very good.
One thing I personally would prefer would be to move this setting out of the GeneralTab to a new PreferenceTab ("Import" or likewise. Creating a new tab should be straightforward, there are plenty of examples). I'm sure, there are other settings in the preferences dialog, that can later be moved there too, to clean the dialog a bit up - but this would not be the focus of this PR.
|
Be aware, that there is still a localization test failing. Please add this string ( |
|
I moved the settings to new Preference tab and moved some of the logic to IdentifierEditorViewModel. The name of the tab is more of a placeholder for the moment. I don't think that "Import" is suitable since this particular setting deals with access to external resources. I still need to update tests to reflect the changes to IdentifierEditor and IdentifierEditorViewModel. |
|
Maybe it suits somewhere else in the preferences dialog? Maybe some more options can later be collected in this new tab? |
|
I think the pull request in the current state is ready for review. The other options I could see being included in the new tab are:
|
Siedlerchr
left a comment
There was a problem hiding this comment.
Thanks for your contribution in general I think this already looks good. Just a few minor remarks
| } | ||
|
|
||
| public void openDOIWithCustomBase(String baseURI) { | ||
| identifier.get().map(x -> (DOI) x).flatMap(x -> x.getExternalURIWithCustomBase(baseURI)).ifPresent( |
There was a problem hiding this comment.
| identifier.get().map(x -> (DOI) x).flatMap(x -> x.getExternalURIWithCustomBase(baseURI)).ifPresent( | |
| identifier.get().map(identifier-> (DOI) identifier).flatMap(DOI::getExternalURIWithCustomBase(baseURI)).ifPresent( |
please no single char variable name like x, use a meaninfgul variable and inside the flatmap you should be able to use a static syntax like DOI::getExternalUriWithCustomBase
There was a problem hiding this comment.
Correct me if I'm wrong but since flatMap requires Function and we have two input arguments (String baseUri and DOI) is it not impossible to implement the static syntax? IntelliJ seems to agree with that.
There was a problem hiding this comment.
Sorry, overlooked that, you are right
| @@ -1,297 +1,159 @@ | |||
| Unable\ to\ monitor\ file\ changes.\ Please\ close\ files\ and\ processes\ and\ restart.\ You\ may\ encounter\ errors\ if\ you\ continue\ with\ this\ session.=Unable to monitor file changes. Please close files and processes and restart. You may encounter errors if you continue with this session. | |||
| %0\ contains\ the\ regular\ expression\ <b>%1</b>=%0 contains the regular expression <b>%1</b> | |||
|
|
|||
There was a problem hiding this comment.
Please revert the deletion of the blank lines, just add the new keys at the bottom
| <CheckBox fx:id="useCustomDOI" text="%Use custom DOI base URI for article access"/> | ||
| <TextField fx:id="useCustomDOIName" HBox.hgrow="ALWAYS"/> | ||
| </HBox> | ||
| </fx:root> No newline at end of file |
There was a problem hiding this comment.
missing newline at the end of the file
calixtus
left a comment
There was a problem hiding this comment.
Thanks for your contribution. Looks good to me to.
One thing I'm not yet sure ist the wording for the customization tab, but this can also maybe changed in some other PR.
Added option in preferences to allow users to choose custom base of DOI address whenever trying to access article via DOI field as requested in #7337 . This is a draft pull request since placement in GUI and functionality are still up for discussion.
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)