Skip to content

Save deletion of current searchquery#2469

Merged
tobiasdiez merged 2 commits into
JabRef:masterfrom
chriba:saveQueryDeletion
Jan 15, 2017
Merged

Save deletion of current searchquery#2469
tobiasdiez merged 2 commits into
JabRef:masterfrom
chriba:saveQueryDeletion

Conversation

@chriba

@chriba chriba commented Jan 15, 2017

Copy link
Copy Markdown
Contributor

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?

@koppor koppor 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.

Besides minor comments: LGTM.

private ContentAutoCompleters autoCompleters;

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

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.

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

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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread CHANGELOG.md Outdated
- 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.

@koppor koppor Jan 15, 2017

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.

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
Copy Markdown
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