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 duplicate external file type issue when change language and other corresponding issue of external file type preferences #10496

Merged
merged 63 commits into from
Dec 17, 2023

Conversation

papatekken
Copy link
Contributor

@papatekken papatekken commented Oct 15, 2023

In Preferences->External file types,

  1. fixes preferences: external file types duplicates #10271, which is about duplicate issue when changing the langauge
  2. EditExternalFileTypeEntryDialog Cancel button close the dialog and wont take further action
  3. Missing value in EditExternalFileTypeEntryDialog will block the closing dialog action when OK button clicked
  4. New External File Type cannot saved if the new file extension exists already

*Notify message was added but no translation done.

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.
  • [x ] 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.

@papatekken papatekken changed the title Fix 10127 Fix duplicate external file type issue when change language and other corresponding issue of external file type preferences Oct 15, 2023
@Siedlerchr
Copy link
Member

There are failing tests
some are for l10n
see https://devdocs.jabref.org/code-howtos/localization.html

@papatekken
Copy link
Contributor Author

thanks for your feedback, I will follow up soon later this week.

@koppor
Copy link
Member

koppor commented Oct 21, 2023

  1. fixed duplicate issue when change langauge #10127

This issue is JabRef/JabRefOnline#2136 - I think, you linked the wrong number. Can you check?

@koppor
Copy link
Member

koppor commented Oct 21, 2023

  1. fixed duplicate issue when change langauge #10127

This issue is JabRef/JabRefOnline#2136 - I think, you linked the wrong number. Can you check?

Ah, it is #10271 (not #10127 - numbers swapped) - I fixed it in the PR descriptoin, too.

@koppor
Copy link
Member

koppor commented Oct 21, 2023

@papatekken Would it be possible to add tests to ExternalFileTypesTabViewModel? With your changes, the ViewModel gets more complicated and should be testet.. - I know, this could be hard, but a good learning of mock frameworks.

@koppor
Copy link
Member

koppor commented Oct 21, 2023

OpenRewrite complains because of style - see the failing "Tests / Code style check" at https://github.com/JabRef/jabref/actions/runs/6593745520/job/17916664973?pr=10496.

Please run ./gradlew rewriteRun to rewrite your code - and check if the resulting code is "better"

@papatekken
Copy link
Contributor Author

@koppor sorry for mix up the number.

I followed the documentation and tried checkStyle plugin locally but all passed.

Anyway I will try to run rewriteRun and see if that can help. I will add the test as you suggested too. Thanks for your comments.

@koppor koppor mentioned this pull request Oct 21, 2023
6 tasks
@koppor
Copy link
Member

koppor commented Oct 21, 2023

I followed the documentation and tried checkStyle plugin locally but all passed.

No worries. -- We have some more steps in there. See

- name: Run OpenRewrite

We did not mention that in the documentation. I am trying to get a bot commenting on the PR to guide more. See #10532.

Updating the documentation is future work :).

@koppor
Copy link
Member

koppor commented Oct 21, 2023

Time is currently shor tin our side. May I ask for a test of ExternalFileTypesTabViewModel (at least of the changes you did)?

@papatekken
Copy link
Contributor Author

Time is currently shor tin our side. May I ask for a test of ExternalFileTypesTabViewModel (at least of the changes you did)?

Sure, I planned to do so, now I target to complete test before Monday hope this is ok, thanks

@papatekken papatekken marked this pull request as ready for review December 11, 2023 01:31
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

  1. Known file type added. I am not able to submit (which is OK). However, there should be an error indicator shown at the text field causing the issue. I think, this is difficult to implement, thus, OK for me.
  2. If I edit an entry and re-use an existing mime-type (2.g., image/vnd.djvu). I can still submit. Mime types are globally unique. Thus, would it be possible to add a check for that?
  3. If I edit an entry - and use an existing name (e.g., CHM), I can still submit. Thus, would it be possible to add a check for that?

Otherwise: Looks good to me!

}
return true;
},
ValidationMessage.error(Localization.lang("There is already an exists extension with the same name."))
Copy link
Member

Choose a reason for hiding this comment

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

Where do I see these messages? I did not see them here? @Siedlerchr Do we need to change something in the overall code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koppor
I following the code from GroupDialogViewModel.java.
In that class there is a function validationHandler. However even with that function I still cannot see the error message, so I didnt include that.

@papatekken
Copy link
Contributor Author

papatekken commented Dec 12, 2023

koppor

No problem, I will add unique validation for name and mime-type as well. For error message, I am not sure at the moment.

@papatekken papatekken marked this pull request as draft December 12, 2023 01:31
@papatekken papatekken marked this pull request as ready for review December 12, 2023 14:16
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Tried it out. Works good. I will ask @Siedlerchr about the validation messages.

@Siedlerchr
Copy link
Member

I was so free to add the missing visualizaion implemention.
Basically you missed the initVisualization for the property/control. This also required me to combine the two validators forreach field, e.g. same name and isNotBlank

grafik

@papatekken
Copy link
Contributor Author

papatekken commented Dec 17, 2023

I was so free to add the missing visualizaion implemention. Basically you missed the initVisualization for the property/control. This also required me to combine the two validators forreach field, e.g. same name and isNotBlank

thanks @Siedlerchr , I get it now, and it's good to learn something new

@Siedlerchr Siedlerchr merged commit 4d86fb4 into JabRef:main Dec 17, 2023
18 checks passed
@papatekken papatekken deleted the fix-10127 branch February 1, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preferences: external file types duplicates
4 participants