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 re-enabling "Jump to entry" for "linked" tokens in Field editors #8456

Closed
wants to merge 3 commits into from

Conversation

k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 commented Jan 23, 2022

Fixes #5484 by enabling "Jump to entry" for,

  • CROSSREF
  • XREF
  • ENTRYSET
  • IDS
  • RELATED
  • XDATA

in the Entry Editor.

I have only verified it for CROSSREF since I don't know how the others are supposed to work. But, since this is re-enabling a feature disabled in #2840 I expect that it is how things used to be.

@tobiasdiez there is a risk this is doing almost exactly what you didn't want in #2840 (directly use the JabRefFrame class).


Todo:


  • 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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr
Copy link
Member

To be honest, I also see no other easy option to get access to the main table or the library tab here.

@tobiasdiez
Copy link
Member

Not sure about easy but the following would work I guess:

  • Pass the stateManager from FieldsEditorTab down to LinkedEntriesEditorViewModel
  • Then in LinkedEntriesEditorViewModel use the setSelectedEntries method of the stateManager to set what entries should be selected
  • For this to work,
    // Poor-mans binding to global state
    stateManager.setSelectedEntries(libraryTab.getSelectedEntries());
    and
    // Add the listener that binds selection to state manager (TODO: should be replaced by proper JavaFX binding as soon as table is implemented in JavaFX)
    mainTable.addSelectionListener(listEvent -> stateManager.setSelectedEntries(mainTable.getSelectedEntries()));
    need to be replaced by a proper bidirectional binding (so that updates of the state manager update the maintable). This is probably a bit more involved but should be doable using the bindContentBidirectional methods in the bindings helper.

@ilippert
Copy link
Contributor

I look so much forward to test this

@Siedlerchr
Copy link
Member

@tobiasdiez I tried to look around as well in this because of these indexOutOfBound exceptions always occurring with regards to selectedItems and the ChangeListener.
In theory your approach about bidirectional binding sounds fine, but in practice this is not possible because the selectedItems is a readOnlyList.

And we never really use it. We just clear the selection in the state manager on tab switching

(I hate the f*cking TableView)

@tobiasdiez
Copy link
Member

Yes you cannot bind it directly, but our helper methods allow custom binding methods and you could use the clearAndSelect method there to transfer changes from the statemanager to the selection model https://docs.oracle.com/javafx/2/api/javafx/scene/control/SelectionModel.html#clearAndSelect(int).

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

It can be done, the problem is that the complexity is number_of_entries * number_of_selection (because when we select multiple entries we have to use their indices) unless we add some extra data structure behind it all.
If the user sorts the table (and there aren't a large number of entries with no value in the column that it is sorted on) we can get log(number_of_entries) * number_of_selection which seems to be the easiest version that might be performant (and is what I was thinking about implementing when I have time)

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

@Siedlerchr where are those IndexOutOfBounds happening?

(I hate the f*cking TableView)

I am starting to understand why 😛

@tobiasdiez
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 I don't really understand your complexity calculation. For what is this the complexity? For keeping the lists in sync?

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member Author

k3KAW8Pnf7mkmdSMPHz27 commented Feb 1, 2022

@tobiasdiez yes. For setting multiple bibentries as selected.

To mark bibentries as selected you need to first find their indices in the (maintable) list

@tobiasdiez
Copy link
Member

Thanks for the clarification. Makes sense. There is also a select method that takes an item (not its index) as the argument, but I'm not sure how this is implemented in the end. But I also wouldn't invest too much energy into finding the most performant implementation as this method is not called that often and only in response to a user action (so it doesn't really matter if it takes a couple of ms longer). That being said, if you have a nice solution, go for it!

@Siedlerchr
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 I was referring to #8394 the exception occurred as well when I switched groups etc, not only when deleting entries.

@koppor
Copy link
Member

koppor commented Feb 18, 2022

DevCall decision:

  • if entry is not available in the main table
    • clear search
    • if still not shown: go grouping hierarchy one level up
    • repeat until shown

@ThiloteE
Copy link
Member

ThiloteE commented May 17, 2022

@Siedlerchr where are those IndexOutOfBounds happening? (#8456 (comment))

See #8719 for a typical example of an index out of bounds issue that may possibly negatively impact this very pull request.

@koppor
Copy link
Member

koppor commented Mar 11, 2024

We fixed the jump to entry in the PR #10578.

@koppor
Copy link
Member

koppor commented Mar 11, 2024

The logic for the UI will be implemented in #11009.

Thus, closing this.

@koppor koppor closed this Mar 11, 2024
@koppor koppor deleted the fix-for-issue-5484 branch September 26, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: 'jump to entry' in 'crossref'
6 participants