JournalAbbreviation search feature

[btut]

I Implemented a string filter for the journal abbreviations tab.
This is not working as of yet. Performing a search currently cleares a list of filtered
abbreviations and fills it back up with abbreviations that contain the
search string. Right now the clearing not only cleares the filtered list
but also the list of all abbreviations and I don't unterstand why.

Fixes #7751

• 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.
  

[btut]

The documentation includes a screenshot of the abbreviations tab. I could update that, but since the feature is not released yet, it's misleading.
Is there a way to create a PR on the documentation repo and mark it to only be merged when the feature is merged/released?

[Siedlerchr]

Some minor code imprvements, otherwise lgtm

[mlep]

The documentation includes a screenshot of the abbreviations tab. I could update that, but since the feature is not released yet, it's misleading.
Is there a way to create a PR on the documentation repo and mark it to only be merged when the feature is merged/released?

I do not know about this, but an alternative could be to alter the documentation while mentioning this is valid for v 5.3 only.

[Siedlerchr]

For the documentation create a PR and we merge it afterwards, but with gitbook I'm unsure if this doesn't break all...

[k3KAW8Pnf7mkmdSMPHz27]

Pretty much everything from my side is nitpicking. View them as suggestions and include them if you agree.

However, I would like you to change JournalAbbreviationsTab.selectNewAbbreviation so that it uses the length of the filtered list instead (perhaps journalAbbreviationsTable.getItems().size() is better).

[k3KAW8Pnf7mkmdSMPHz27]

However, I would like you to change JournalAbbreviationsTab.selectNewAbbreviation so that it uses the length of the filtered list instead (perhaps journalAbbreviationsTable.getItems().size() is better).

I meant

jabref/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTab.java

Lines 137 to 141 in f2b51fb

	private void selectNewAbbreviation() {
	int lastRow = viewModel.abbreviationsCountProperty().get() - 1;
	journalAbbreviationsTable.scrollTo(lastRow);
	journalAbbreviationsTable.getSelectionModel().select(lastRow);
	journalAbbreviationsTable.getFocusModel().focus(lastRow);

GitHub doesn't allow me to comment on code that is that far out of the region of your changes 😝

Since abbreviationsCountProperty uses the non-filtered list. Since the filtered list is always equal or smaller, I don't believe it can throw an error and, at least for now, works.

[btut]

However, I would like you to change JournalAbbreviationsTab.selectNewAbbreviation so that it uses the length of the filtered list instead (perhaps journalAbbreviationsTable.getItems().size() is better).

I meant

jabref/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTab.java

Lines 137 to 141 in f2b51fb

	private void selectNewAbbreviation() {
	int lastRow = viewModel.abbreviationsCountProperty().get() - 1;
	journalAbbreviationsTable.scrollTo(lastRow);
	journalAbbreviationsTable.getSelectionModel().select(lastRow);
	journalAbbreviationsTable.getFocusModel().focus(lastRow);

GitHub doesn't allow me to comment on code that is that far out of the region of your changes stuck_out_tongue_closed_eyes

Since abbreviationsCountProperty uses the non-filtered list. Since the filtered list is always equal or smaller, I don't believe it can throw an error and, at least for now, works.

Ah, I did not see that. But what if the new abbreviation does not match the search criteria? Then setting the selection to the last row in the search result is not very useful. Would it maybe make sense to clear the search before adding new abbreviations?

[k3KAW8Pnf7mkmdSMPHz27]

Ah, I did not see that. But what if the new abbreviation does not match the search criteria? Then setting the selection to the last row in the search result is not very useful. Would it maybe make sense to clear the search before adding new abbreviations?

Huh, I hadn't thought of that 🤔
I think your suggestion is good. On the one hand, I dislike disappearing search text, but on the other hand, that seems far better than not giving visual feedback on an added abbreviation. @Siedlerchr do you know of anything?

[Siedlerchr]

What about adding a label e..g No search results found?

[k3KAW8Pnf7mkmdSMPHz27]

@Siedlerchr the use case is,

1. You have filtered the list, (say you only want IEEE)
2. You are adding a new Abbreviation that has nothing to do with IEEE and should therefore not be shown in the filtered list
3. After the abbreviation is added, the current behavior would select the last item in the filtered list (even if the last item is not the added abbreviation)

[Siedlerchr]

Ah, now I understand. I would simply not select anything after adding, the same as when you search for something and add a new entry to the table. The new entry is not shown because you have filtered the list and as far as I remember it's also not selected

[btut]

Ah, now I understand. I would simply not select anything after adding, the same as when you search for something and add a new entry to the table. The new entry is not shown because you have filtered the list and as far as I remember it's also not selected

Are you talking about the main bib-table? It's a little different there as when you add an entry, the entryeditor is opened so there is some visual feedback that an entry was added and users are able to edit them.
For journal abbreviations, there is no such thing. They are edited directly in the table. So from a users perspective, I would probably click the 'add' button several times if no new entry shows up. I would prefer jabref losing my search term rather than my new entry.

[koppor]

+1 for clearing the search when adding a new abbreviation.

[btut]

As discussed yesterday with @koppor and @calixtus, I added visual feedback when invalidating the search when adding a new abbreviation. The search-bar now will briefly flash in red color.

[calixtus]

Thanks for integrating the flash. This should make it now very obvious that the search has been cleared. UI looks very good to me. I will take a closer look at the source tonight.

[k3KAW8Pnf7mkmdSMPHz27]

Looks good to me! Change the SimpleStringProperty and it should be good to go.

---

There are still some issues with which Abbreviation gets selected and how, but I'd consider it unrelated to this PR. Perhaps a follow-up issue? Some of it might be beginner friendly.

One example of this is

1. Create a new Abbreviation
2. The first column (with default text Name should be selected)
3. Click on an empty spot of the table
4. Press enter and get a Cannot invoke "javafx.scene.control.TableColumnBase.isEditable()" because the return value of "javafx.scene.control.TablePositionBase.getTableColumn()" is null message (most likely we are selecting the full row rather than the cell, I haven't verified this)

Another seem to be that there always is an empty row in a custom abbreviations list.

[btut]

There are still some issues with which Abbreviation gets selected and how, but I'd consider it unrelated to this PR. Perhaps a follow-up issue? Some of it might be beginner friendly.

One example of this is

1. Create a new Abbreviation

2. The first column (with default text `Name` **should** be selected)

3. Click on an empty spot of the table

4. Press enter and get a `Cannot invoke "javafx.scene.control.TableColumnBase.isEditable()" because the return value of "javafx.scene.control.TablePositionBase.getTableColumn()" is null` message (most likely we are selecting the full row rather than the cell, I haven't verified this)

Selecting the right column (add2d44) does not completely fix this, but gets rid of the exception. Hitting enter keeps the text-field in the name column selected, but does not commit it. One has to click the text again and hit enter to apply the changes.

[btut]

Speaking of the selection, I don't know how safe it is to do the selection right after starting the animation for the search-invalidation. Maybe it would be better to do all the actions for the new abbreviation (addAbbreviation, selectNewAbbreviation, editAbbreviation) only after the search is invalidated? Just to make sure the abbreviation is visible when doing the selection?

Edit: Tried it out, behaviour is still the same but I think it is safer like this. 4cf14b7

[Siedlerchr]

I would have expected to have the search field above the table, what do the others think?

[calixtus]

As the line above the list is already crowded with various controls, I would vote to merge this PR as it is now and redesign the whole dialog in another PR.

[calixtus]

Thanks!