Skip to content

Fixed switching off autocomplete on specific fields#7496

Merged
calixtus merged 6 commits into
JabRef:masterfrom
BJaroszkowski:fix-for-issue-7320
Mar 8, 2021
Merged

Fixed switching off autocomplete on specific fields#7496
calixtus merged 6 commits into
JabRef:masterfrom
BJaroszkowski:fix-for-issue-7320

Conversation

@BJaroszkowski

Copy link
Copy Markdown
Contributor

This fixes issue described in #7320. Even though user could choose fields on which autocomplete should not work it had no actual effect.

  • 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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Comment thread src/test/java/org/jabref/gui/autocompleter/FieldValueSuggestionProviderTest.java Outdated

@Siedlerchr Siedlerchr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 7, 2021
BJaroszkowski and others added 2 commits March 7, 2021 14:39
Co-authored-by: Christoph <siedlerkiller@gmail.com>

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @BJaroszkowski . Especially thanks for providing also a test for your change. Thanks for your contribution so far.
One small suggestion and a bigger request: Please do not move stuff around, if they are not directly related to changes. This easily leads merge errors, if others are working on the class (in this case LibraryTab) too, makes it more complicated to review and there is also in many classes some kind of order we try to keep with the definitions to make it easier to read and understand. So please undo the unnecessary changes in LibraryTab.java

Comment on lines +72 to +73
BibEntry entry = new BibEntry();
entry.setField(StandardField.TITLE, "testValue");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

new BibEntry().withField()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have to admit I was not aware of existence of this method, this would certainly be an improvement. The thing is that all the tests are written in the manner I did it. It would probably be a good idea to change them all at once for the sake of consistency. This however sounds like something for another PR.

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

I think the changes in the LibraryTab come from (automatically) running the code formatter. Should be fine.

@BJaroszkowski

Copy link
Copy Markdown
Contributor Author

Yes, this is exactly the case. Should I roll back the changes anyway?

@calixtus

calixtus commented Mar 8, 2021

Copy link
Copy Markdown
Member

Nope, I checked the currently open PRs in the issue tracker and think there is none, that depends on LibraryTab, so I guess we can merge. JabRefFrame and LibraryTab need some refactoring anyway.

@calixtus calixtus merged commit 2dc0d5b into JabRef:master Mar 8, 2021
@BJaroszkowski BJaroszkowski deleted the fix-for-issue-7320 branch March 8, 2021 20:54
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.

4 participants