Skip to content

[GSOC22] - D - Test the Three Way Merge UI#9069

Merged
Siedlerchr merged 30 commits into
mainfrom
GSOC22-Test-Three-Way-Merge
Sep 10, 2022
Merged

[GSOC22] - D - Test the Three Way Merge UI#9069
Siedlerchr merged 30 commits into
mainfrom
GSOC22-Test-Three-Way-Merge

Conversation

@HoussemNasri

@HoussemNasri HoussemNasri commented Aug 17, 2022

Copy link
Copy Markdown
Member

Besides unit testing the three-way merge UI, the code was refactored for better testability

  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • 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.

@HoussemNasri HoussemNasri added dev: code-quality Issues related to code or architecture decisions component: duplicate-finder project: gsoc labels Aug 17, 2022
@HoussemNasri HoussemNasri changed the title [WIP][GSOC22] - D - Test Three Way Merge UI [WIP][GSOC22] - D - Test the Three Way Merge UI Aug 17, 2022
Siedlerchr
Siedlerchr reviewed Aug 22, 2022
View reviewed changes
Comment thread src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/GroupMerger.java Outdated
@HoussemNasri
Extract regex into a field of type Pattern
6c5131d
@HoussemNasri
Improve groups splitting performance
3452a95
@HoussemNasri
Delete AbstractCell.java
511dab7
@HoussemNasri
Rename 'NO_ROW_INDEX' to 'HEADER_ROW'
567c71d 
- Because NO_ROW_INDEX is passed to ThreeWayMergeCell for header cells only
@HoussemNasri
Extract logic from ThreeWayMergeCell to a view model
19a36e5
@HoussemNasri
Use CSS to style cell padding
45a66aa
@HoussemNasri
Test ThreeWayMergeCellViewModel
3ce56ea
@HoussemNasri
Merge remote-tracking branch 'upstream/main' into GSOC22-Test-Three-W…
92aaf8e 
…ay-Merge
@HoussemNasri
Merge branch 'main' into GSOC22-Test-Three-Way-Merge
da7d622
@HoussemNasri
Don't override setUserData() and getUserData()
a12bdd6 
- Setting user data was useful before when FieldRow depended on it to figure out the value of the selected cell, but now it has a different implementation
@HoussemNasri
Create FieldValueCellViewModel
d5dee77
@HoussemNasri
Refactor OpenExternalLinkAction
ef95c17
@HoussemNasri
Refactor ThreeWayMergeViewModel
6bb5092
github-actions[bot]
github-actions Bot reviewed Aug 30, 2022
View reviewed changes
Comment thread src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java Outdated
Comment thread src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java Outdated
Comment thread src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java Outdated
@HoussemNasri HoussemNasri marked this pull request as ready for review August 31, 2022 17:17
calixtus
calixtus approved these changes Sep 6, 2022
View reviewed changes

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

LGTM, just one question about a comment

Comment thread src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java Outdated
github-actions[bot]
github-actions Bot reviewed Sep 6, 2022
View reviewed changes
Comment thread src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java
@calixtus calixtus requested a review from Siedlerchr September 10, 2022 00:17
@HoussemNasri

HoussemNasri commented Sep 10, 2022

Copy link
Copy Markdown
Member Author

ThemeManagerTest#installThemeOnScene is failing. It throws a java.lang.IllegalStateException: Called endChange before beginChange. According to this comment, this exception is thrown when a list is modified from multiple threads. In our case, this list is the scene's stylesheets. I have never seen this exception while interacting with JabRef, nor while testing, so I think testfx might have caused it, but I need to dig more into it to confirm.

@calixtus

calixtus commented Sep 10, 2022

Copy link
Copy Markdown
Member

It's hiccup if a more deeper issue with the stylesheet management of javafx and how the ThemeManager interacts with it. I don't think you should bother with it in this PR. As for now the tests are all green. So if @Siedlerchr is ok with this, we should merge this PR soon.

@Siedlerchr Siedlerchr changed the title [WIP][GSOC22] - D - Test the Three Way Merge UI [GSOC22] - D - Test the Three Way Merge UI Sep 10, 2022
@Siedlerchr

Siedlerchr commented Sep 10, 2022

Copy link
Copy Markdown
Member

Yep, looks good so far. I would not spend time on this ThemeManager issue. As long as it works when running JabRef it's not worth spending time on it. Maybe we can disable the test on CI

@Siedlerchr Siedlerchr merged commit 3867072 into main Sep 10, 2022
@Siedlerchr Siedlerchr deleted the GSOC22-Test-Three-Way-Merge branch September 10, 2022 20:58
Siedlerchr added a commit to LIM0000/jabref that referenced this pull request Sep 12, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fix-for-issue-8189
b81c1a5 
* upstream/main:
  Fix missing CSS for some dialogs (JabRef#9150)
  Bump org.eclipse.jgit from 6.2.0.202206071550-r to 6.3.0.202209071007-r (JabRef#9152)
  Bump mockito-core from 4.7.0 to 4.8.0 (JabRef#9151)
  [GSOC22] - D - Test the Three Way Merge UI (JabRef#9069)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@calixtus calixtus calixtus approved these changes

Assignees

No one assigned

Labels

component: duplicate-finder dev: code-quality Issues related to code or architecture decisions project: gsoc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HoussemNasri @calixtus @Siedlerchr