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

Fixing Group color not imported after closing the app #7096

Closed
wants to merge 2 commits into from
Closed

Fixing Group color not imported after closing the app #7096

wants to merge 2 commits into from

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Nov 14, 2020

  • Hi, I found the bug Group's color not saved #6999, it's caused by org.jabref.logic.importer.util.GroupParser.searchGroupFromString(), the problem is when parsing a search group most of the properties gets parsed by searchGroupFromString() but some properties like (expanded, color, iconName, description) gets parsed by another method addGroupDetails()

  • to fix it instead of returning searchGroup right away i called addGroupDetails() on searchGroup to add the missing properties then I returned searchGroup

  • Fixes Group's color not saved #6999

  • Change in CHANGELOG.md described (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.

@HoussemNasri HoussemNasri changed the title Fixing SearchGroup color not updated after closing the app Fixing Group color not updated after closing the app Nov 14, 2020
@HoussemNasri HoussemNasri changed the title Fixing Group color not updated after closing the app Fixing Group color not imported after closing the app Nov 14, 2020
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this issue and finding a solution! Most welcome!
Codewiese looks good to me!

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 14, 2020
@HoussemNasri
Copy link
Member Author

@Siedlerchr Thanks for looking into my code, I am new to open source and git & github so i have some questions if you don't mind:

  • there is 3 checks are failing in my PR, is there something i did wrong?
  • I didn't edit the CHANGELOG.md, will that be a problem?

@Siedlerchr
Copy link
Member

@HoussemNasri Thanks for chosing JabRef! Your contirbution is most welcome 😼
The Deployent for mac will always fail for forks, because the GitHub Action has some secrets defined (some keys required for signing) which are not available in forks. This is some security thing.

If you didn't work on a fetcher, you can ignore the fetcher tests. They are often failing because of timeouts or that the Gihub Runner is blocked.
Unit Test + Checkstyle are the two most important ones.

Regarding Changelog: The Changelog is for the users to see what is fixed in each version. Therefore, just add a line describing which problem is fixed now. Just add another commit and the PR will be udpated automatically.

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to JabRef! Your changes look good to me.

@koppor Author test fails for some reason, with the following text:

The JabRef maintainers will add the following name to the AUTHORS file
Houssem
Houssem Nasri
HoussemNasri

I guess the second one is correct. Not sure what to do, so I let you merge.

@HoussemNasri
Copy link
Member Author

HoussemNasri commented Nov 14, 2020

@tobiasdiez yes it's the second one, I think this happened because I changed my user.name in git

@koppor
Copy link
Member

koppor commented Nov 14, 2020

Squashed-and-Merged using the command line. @HoussemNasri Thank you for the contribution

@koppor koppor closed this Nov 14, 2020
@HoussemNasri HoussemNasri deleted the fix-for-issue-6999 branch November 14, 2020 20:35
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

I believe this closes #6999 (which has not happened automatically)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group's color not saved
5 participants