Made the empty cells in the Original entry selectable in multiwaymerge#15267
Made the empty cells in the Original entry selectable in multiwaymerge#15267ZiadAbdElFatah wants to merge 2 commits into
multiwaymerge#15267Conversation
| } else { | ||
| viewModel.mergedEntryProperty().get().setField(field, ((DiffHighlightingEllipsingTextFlow) ((ToggleButton) newValue).getGraphic()).getFullText()); | ||
| headerToggleGroup.selectToggle(null); | ||
| } |
There was a problem hiding this comment.
Now I added the ability to select the empty cells, but when I select the empty cell, the field in the merged entry doesn't change.
I tried many workarounds here, such as clearField() and setField(""), but they all failed.
I also can't totally confirm my tries to solve it because of this issue #15265.
✅ All tests passed ✅🏷️ Commit: e637439 Learn more about TestLens at testlens.app. |
| MultiMergeEntriesViewModel.EntrySource originalSource = viewModel.entriesProperty().get().getFirst(); | ||
| BibEntry originalEntry = originalSource.entryProperty().get(); | ||
| if (originalEntry.getField(newField).isEmpty()) { | ||
| UiTaskExecutor.runInJavaFXThread(() -> { | ||
| Cell emptyCell = new Cell("", newField, 0); | ||
| optionsGrid.add(emptyCell, 0, fieldRow.rowIndex); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This code is inside the if - there is AND fieldRows.containsKey. -- if the row already exists, there is no cell created.
Try to program a reproducer.
Try to craft an MWE - calling only the dialog in a @Test with mocked properties. - You can run the test then. Later, add @Disabled and comment: "Manual testing only".
There was a problem hiding this comment.
This code is inside the if - there is AND fieldRows.containsKey. -- if the row already exists, there is no cell created.
Is it even possible that a row for the merged entry can exist before this block of code? I don't think so.
I think the field rows will always be empty before executing this block of code as this is the first code block to deal with it.
I would like to know if you see an edge case or a scenario I can't imagine.
There was a problem hiding this comment.
Then draw a sequence diagram - https://mermaid.ai/open-source/syntax/sequenceDiagram.html
Focus on showing what happens if the second entry is added.
Focus on only two entries.
There was a problem hiding this comment.
Here is the sequence diagram showing how merging happens for two entries. Marking cells in the original entry as empty is always the last step.
There was a problem hiding this comment.
What do you think of the sequence @koppor?
There was a problem hiding this comment.
Sequence diagram seems to be incomplete. The decision which field is available at merged entries is not shown.
There was a problem hiding this comment.
Ok, I tried to make it clearer.
And here is an explanation of the decision, which field is available at the merged entry:
- The original entry adds all its fields and selects them in the merged entry.
- Another original tries to add its fields. Now, there are two scenarios:
- A field already exists and is selected in the merged entry, this field will not be updated and will only be an option for the user to update it.
- A field that doesn't exist in the merged entry, it will be added and selected in the merged entry.
|
The requested changes were not addressed for 10 days. Please follow-up in the next 10 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
|
I don't know about the state of the PR. Does it work or not? If not, some assumptions are wrong. I think, my comment #15267 (comment) is still valid / the discussion regarding the UML sequence diagram. The refinement of the diagram could help. |
It works and achieves part of the issue it intends to solve, which is: The part still doesn't work (I am still waiting for your confirmation on my current approach to know if I should continue with this approach, or if I have to change it), is: The problem in selection may also need a separate issue.
I provided an updated version of the diagram with an explanation of the decision in this comment, hope it will clarify the PR statement more. |
Related issues and pull requests
Closes #15014
PR Description
Added the ability to select empty cells from Original entry in
multiwaymergeSteps to test
Now the empty cells of Original can be selected.
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)