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 a few group related issues #2361

Merged
merged 26 commits into from
Dec 11, 2016
Merged

Fix a few group related issues #2361

merged 26 commits into from
Dec 11, 2016

Conversation

tobiasdiez
Copy link
Member

@tobiasdiez tobiasdiez commented Dec 10, 2016

This PR fixes #2334, #1873 and #1681.
Since I didn't understood the groups code, I also refactored it. Main changes:

  • Split KeywordGroup class into WordKeywordGroup, which splits field content into words and then matches them, and RegexKeywordGroup that uses a regular expression for this job.
  • Extract serialization of groups to GroupSerializer (previously every group decided about its own serialization via the toString method)
  • Move a few things from model and logic to gui since they mostly contained ui-related stuff (e.g. GroupDesrciptions)

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 10, 2016
@lenhard
Copy link
Member

lenhard commented Dec 11, 2016

The groups code is a mess :-( Thanks for being willing to put up with this!

Unfortunately, the amount of changes is too large for meaningful review and my knowledge on groups and the related code is too limited. I would trust your ability, and the amount of tests that you added here together with the working build. Cosmetic changes can be made on a later occasion. I am sure this is not the last visit to the groups code.

Since this is on your private branch, we have no build on builds.jabref.org. If we had, we could ask the people that reported the issues to try out this version. Any chance of getting a build for this?

<?xml version="1.0" encoding="UTF-8"?>
<Diagram>
<ID>JAVA</ID>
<OriginalElement>net.sf.jabref.model.groups.AbstractGroup</OriginalElement>
Copy link
Member

Choose a reason for hiding this comment

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

Why not using PlantUML for documenting?

Which tool did you use?

Copy link
Member Author

@tobiasdiez tobiasdiez Dec 11, 2016

Choose a reason for hiding this comment

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

IntelliJ's built-in one

@koppor
Copy link
Member

koppor commented Dec 11, 2016

Decision: merge so that many people will be able to test it. We can include code review comments later in a separate PR.

I am very aware of the risk. Refs #1495.

@koppor
Copy link
Member

koppor commented Dec 11, 2016

  • dragging an entry to a group does not refresh the main table. (keyword-based group)
  • context menu shows "Remove group, keep subgroups" even though there are no subgroups
  • deletion of a static group does not remove the assignment of entries to it
  • clicking on a group and pressing Del triggers the dialog "Do you want to remove the selcted entries". If I press Cursor Down, the next group is selected. Thus Del should really apply to the current group.

Nothing important or me. Think, this was also happening using the old groups code.

@koppor koppor merged commit 30ba6b6 into JabRef:master Dec 11, 2016
@tobiasdiez tobiasdiez deleted the fixGroups branch December 12, 2016 09:32
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.

"Remove entry selection from this group" removes entries from similarly named groups
3 participants