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 order of string in editor #10083

Merged
merged 6 commits into from
Jul 15, 2023

Conversation

stephanlukasczyk
Copy link
Contributor

@stephanlukasczyk stephanlukasczyk commented Jul 14, 2023

The string defined in a BibTeX file were shown in random order in the respective GUI editor. While the code already sorts them, by storing the items into a set, it destroys the order again. Fix this behaviour by storing them as a list, which retains the order.

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.

The string defined in a BibTeX file were shown in random order in the
respective GUI editor. While the code already sorts them, by storing the
items into a set, it destroys the order again. Fix this behaviour by
storing them as a list, which retains the order.
@stephanlukasczyk stephanlukasczyk marked this pull request as ready for review July 14, 2023 15:10
BibtexString string1 = new BibtexString("TSE", "Transactions on Software Engineering");
BibtexString string2 = new BibtexString("ICSE", "International Conference on Software Engineering");
Collection<BibtexString> stringValues = List.of(string1, string2);
when(db.getStringValues()).thenReturn(stringValues);
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to mock the database here, you can simply create a new object without mocking e.g.

bibDatabaseContext = new BibDatabaseContext(new BibDatabase());


ListProperty<ConstantsItemModel> strings = model.stringsListProperty();
assertAll(
() -> assertEquals("ICSE", strings.get(0).labelProperty().getValue()),
Copy link
Member

Choose a reason for hiding this comment

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

You can direclty compare the collections like this:

     var strings  = model.stringsLIstPrroperty.stream().map(ConstantsItemModel::labelProperty).map(StringProperty::getValue).collect(Collectors.toList());

Just assertEquals(stringValues, strings)

@Siedlerchr
Copy link
Member

Thanks for the bugfix and for the creation of the test, just some minor "nitpicks" for improving the code style

@stephanlukasczyk
Copy link
Contributor Author

Thanks for the bugfix and for the creation of the test, just some minor "nitpicks" for improving the code style

Thanks for the quick review, I've attempted to address both suggestions. Shall I squash the two commits or is it fine to leave them/done automatically during merging?

@Siedlerchr
Copy link
Member

Just push your new changes, we squash them when merging!

Siedlerchr
Siedlerchr previously approved these changes Jul 14, 2023
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 14, 2023
@calixtus
Copy link
Member

Codewise looks good, but i mentioned that after adding a new string, the list is probably not sorted again.
Can you check that please?

@stephanlukasczyk
Copy link
Contributor Author

Codewise looks good, but i mentioned that after adding a new string, the list is probably not sorted again. Can you check that please?

I've just checked, the list is not sorted after adding a new string. This will only be done by hitting the apply button and afterwards reopening the dialogue. However, I would argue that this is an acceptable behaviour here IMHO: if the list would resort as soon as one finishes to add an entry, the new entry might get lost from the immediate focus of the user if one has a larger list of strings. One could, of course, keep the focus on the newly created entry, but that would require probably a larger change in the whole dialogue.

@calixtus
Copy link
Member

calixtus commented Jul 15, 2023

Took a quick look, would be nice if you could quickly implement this: Instead of void return the newly added item from ConstantsPropertiesViewModel#addNewItem and in ConstantsPropertiesView#addString resort after adding the string and use the fxcontrols SelectionModel to select the newly created item.

Adding a new entry should also preserve the sorting of the strings
table, thus we need to resort the entries and take care that the focus
in the table is still on the newly-created entry.
@stephanlukasczyk
Copy link
Contributor Author

I've implemented the requested reordering @calixtus in 90adb53. However, I did it a bit different than you've suggested: it's now a hook to the key column of the table. When adding a new key, the table is resorted and the UI preserves the focus on the newly-created item. I hope, this fulfills your idea, I am not really into JavaFX.

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.

LGTM 👍. The changes need a second review and thus this PR awaits a second review.

@Siedlerchr Siedlerchr merged commit bd03fbe into JabRef:main Jul 15, 2023
11 of 12 checks passed
@Siedlerchr
Copy link
Member

Thanks again for your contribution! Looking forward to more ;)

@stephanlukasczyk stephanlukasczyk deleted the fix-string-list-ordering branch July 15, 2023 10:29
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.

3 participants