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 the exception when export saving order is less than 3 in export tab of preferences. #10343

Merged
merged 10 commits into from
Sep 10, 2023

Conversation

papatekken
Copy link
Contributor

When saving the Preferences, if no. of items in export tab->saving order list is less than 3, exception is raised.

So in this change, it will check the no. of items in saving order list before save to corresponding prerferences attribute.

That will fix #10157

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.

@papatekken papatekken changed the title Fix raising exception when export saving order is less than 3 in export tab of preferences. Fix the exception when export saving order is less than 3 in export tab of preferences. Sep 7, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 8, 2023
Siedlerchr
Siedlerchr previously approved these changes Sep 8, 2023
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Overall looks good, just the changelog and then it's good from my side

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 out locally. Works well.

Nitpick remaining ^^.

@papatekken papatekken requested a review from koppor September 9, 2023 16:44
CHANGELOG.md Outdated Show resolved Hide resolved
Siedlerchr
Siedlerchr previously approved these changes Sep 9, 2023
@Siedlerchr
Copy link
Member

Thanks for the update! looks good

calixtus
calixtus previously approved these changes Sep 10, 2023
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.

Does not look very beautiful, but I guess it's the correct and there is no nicer way to do this. Therefor looks 'good' to me 😄

@calixtus calixtus enabled auto-merge September 10, 2023 15:23
@Siedlerchr Siedlerchr dismissed stale reviews from calixtus and themself via 5df0f55 September 10, 2023 16:01
@papatekken
Copy link
Contributor Author

Does not look very beautiful, but I guess it's the correct and there is no nicer way to do this. Therefor looks 'good' to me 😄

I agree it does not look very nice.
To do it in a better way it may need a bigger change with the data structure for the saving order preference, and the current preference export file need change too.

@Siedlerchr Siedlerchr disabled auto-merge September 10, 2023 16:31
@Siedlerchr Siedlerchr merged commit 0cf4acc into JabRef:main Sep 10, 2023
13 checks passed
@papatekken papatekken deleted the fix-10157 branch September 24, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

export sort order: use specified order
4 participants