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

Save deletion of current searchquery #2469

Merged
merged 2 commits into from
Jan 15, 2017
Merged

Save deletion of current searchquery #2469

merged 2 commits into from
Jan 15, 2017

Conversation

chriba
Copy link
Contributor

@chriba chriba commented Jan 15, 2017

Fixes #2468.

If one deleted the current query it was not saved (every basepanel can have it's own query).

The only neede change is the line in GlobalSearchBar.java.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

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.

Besides minor comments: LGTM.

@@ -207,7 +207,8 @@

private ContentAutoCompleters autoCompleters;

private SearchQuery currentSearchQuery;
/** the query the user searches when this basepanel is active */
Copy link
Member

Choose a reason for hiding this comment

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

Please use // as comment for fields. (Or quote some manual where block comments are recommended)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JavaDoc works on properties too (I think you overlooked the second asterisks).

public void setCurrentSearchQuery(SearchQuery currentSearchQuery) {
this.currentSearchQuery = currentSearchQuery;
if (currentSearchQuery == null) {
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 use Optional.ofNullable(currentSearchQuery) instead of this if/else block

Copy link
Member

Choose a reason for hiding this comment

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

However, I am unsure whether trim().isEmpty() should be checked for. I think, your current code is OK. (After the usage of ofNullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -19,6 +19,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
- Non-ISO timestamp settings prevented the opening of the entry editor (see [#2447](https://github.com/JabRef/jabref/issues/2447))
- When pressing <kbd>Ctrl</kbd> + <kbd>F</kbd> and the searchbar is already focused the text will be selected.
- LaTeX symbols are now displayed as Unicode for the author column in the main table. "'n" and "\'{n}" are parsed correctly. Fixes [#2458](https://github.com/JabRef/jabref/issues/2458).
- Fixes [#2468](https://github.com/JabRef/jabref/issues/2468): Save deletion of current searchquery.
Copy link
Member

@koppor koppor Jan 15, 2017

Choose a reason for hiding this comment

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

I would use (a slight update of) your long sentence of the PR text here - and put the "Fixes" as last (as with the other entries)

If one deleted the current query, it was not saved (every database can have its own query). Fixes #2468.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tobiasdiez tobiasdiez merged commit 9e48a5f into JabRef:master Jan 15, 2017
@chriba chriba deleted the saveQueryDeletion branch January 15, 2017 23:30
Siedlerchr added a commit that referenced this pull request Jan 18, 2017
* upstream/master:
  Save deletion of current searchquery (#2469)
  Update dev dependencies (mockito, wiremock, assertj)
  Update BouncyCastle (1.55->1.56), ANTRL4 (4.5.3->4.6), jsoup (1.10.1->1.10.2)
  Group all checker which only check the value of one field (#2437)
  Follow up #2428 (#2438)
  Fix conversion of "'n" and "\'{n}" from LaTeX to Unicode (#2464)
  Fix failing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants