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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 30 commits into from
Sep 10, 2022
Merged

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

merged 30 commits into from
Sep 10, 2022

Conversation

HoussemNasri
Copy link
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 type: code-quality Issues related to code or architecture decisions duplicateFinder 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
@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
@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
Copy link
Member

@calixtus calixtus left a comment

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

src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java Outdated Show resolved Hide resolved
@HoussemNasri
Copy link
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
Copy link
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
Copy link
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 github-actions left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@calixtus calixtus calixtus approved these changes

Assignees
No one assigned
Labels
duplicateFinder project: GSoC type: code-quality Issues related to code or architecture decisions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HoussemNasri @calixtus @Siedlerchr