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 845 #930

Merged
merged 6 commits into from
Mar 13, 2016
Merged

Fix 845 #930

merged 6 commits into from
Mar 13, 2016

Conversation

simonharrer
Copy link
Contributor

  • better German translation of the menu items groups -> highlight groups ...
  • add radio button functionality to the menu items groups -> highlight groups ... which is mutually exclusive plus add a disable menu entry which was not explicit before
  • enable showing what is currently toggled in the menu as well (and not only in the toolbar)

@oscargus
Copy link
Contributor

Looks good!

@koppor
Copy link
Member

koppor commented Mar 10, 2016

Code looks good.

I don't see the current state in the toolbar. - The radio indicators change. I don't see any change in my database, but that was also the case before your change.

@simonharrer
Copy link
Contributor Author

Change in the database? Why should there be a change in the database?

What are you not seeing in the toolbar? All toolbar stuff works on my machine.

@koppor
Copy link
Member

koppor commented Mar 11, 2016

Yeah, only the group window should have a change.

Could we implement "highlight" as change in the background color (maybe the JabRef colour) and not via underline? 😇 -> Filed as #932

Where in the toolbar should I see something?

@simonharrer
Copy link
Contributor Author

I do not want to mess with the groups code at the moment, as this is done by @tobiasdiez currently - avoding any merge conflicts.

The toolbar has not been changed, but the menu itself now shows the state of the toogle-actions.

@tobiasdiez
Copy link
Member

Please rebase, then this can be merged in my opinion.
@koppor can you please open a new issue for your suggestion regarding highlighting.

…works with an additional disable radio button in the menu plus using only a single enum (i.e., a string value instead of using two boolean flags with forbidden combinations (true, true))
@simonharrer
Copy link
Contributor Author

I had to improve this fix: previously, two boolean preferences were used to describe the three states DISABLED, ANY and ALL. The combination (true, true) of the booleans was not allowed. I replaced this by using a single string preference which is either "all", "any" or anything else which resorts to DISABLED. I did not implement any migrations as I would take it into account if the behaviour changes upon upgrade as the user can easily change the setting manually. Should not be a hassle.

@simonharrer simonharrer added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers bug Confirmed bugs or reports that are very likely to be bugs type: enhancement labels Mar 11, 2016
@tobiasdiez
Copy link
Member

Would it make sense to create an enum for these states?

@simonharrer
Copy link
Contributor Author

I tried to create an enum, but ended up creating a separate preferences class with methods for each setting.

simonharrer added a commit that referenced this pull request Mar 13, 2016
@simonharrer simonharrer merged commit 96a5471 into master Mar 13, 2016
@stefan-kolb stefan-kolb deleted the fix-845 branch March 13, 2016 11:21
@koppor
Copy link
Member

koppor commented Mar 13, 2016

I wonder whether MDSD would have been of any help here 😇

@simonharrer
Copy link
Contributor Author

???

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

Successfully merging this pull request may close these issues.

4 participants