Conversion of group dialog to mvvm-pattern #5826
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t RadioButtons to a ComboBox
# Conflicts: # src/main/java/module-info.java
|
👍 for removing the description. |
|
So I got no other opinions. Descriptions are gone.
refs #5833, but this PR does only change the groups dialog, it does not fix the issue mentioned. |
calixtus
added
the
status: ready-for-review
# Conflicts: # src/main/resources/l10n/JabRef_en.properties
calixtus
changed the title
|
Looks good so far! |
Siedlerchr
reviewed
Jan 19, 2020
Siedlerchr
reviewed
Jan 19, 2020
# Conflicts: # src/main/resources/l10n/JabRef_en.properties
tobiasdiez
approved these changes
Jan 25, 2020
# Conflicts: # src/main/resources/l10n/JabRef_en.properties
|
The constructors are gone. And the whole Globals-stuff in this part probably too. As I made same bigger changes, I would appreciate it, if you could just take a quick look at the latest commit and the changes to the optics again. Expecially the EnumMap. Is this ok this way? I really did not like the getText-Method... |
tobiasdiez
approved these changes
Jan 25, 2020
There was a problem hiding this comment.
Looks fine to me.
From the conceptual level, the cleanest solution would be to introduce a class GroupHierachyTypeViewModel which is a wrapper around GroupHierarchyType and enriches it by the display text and tooltip. However, this feels also a bit over-engineering too me. I guess both approaches via ordinary methods or via the enum map are fine.
|
Merging then... |
koppor
pushed a commit
that referenced
this pull request
Jan 15, 2022
5563ccc Update american-society-for-microbiology.csl (#5842) 19ab80f Merge pull request #5843 from POBrien333/patch-1002 d4a6c6d double trouble 424f7fe in-text >> superscript for T&F ACS style 605253c Merge pull request #5837 from POBrien333/patch-998 eb5417d Merge pull request #5838 from alessandro-gentilini/patch-1 7d2d3f5 Fix typo 9f557b5 Re-indent CSL styles f587e60 polyglot style 3a3fe2c Create journal-fur-kulturpflanzen-journal-of-cultivated-plants.csl cb24633 Merge pull request #5826 from dstark/patch-7 2590a6c Merge pull request #5833 from POBrien333/patch-997 5b2481e Create nist-techpubs-jres.csl (#5756) d2a1a49 Re-indent CSL styles 42c51d1 Update tyndale-bulletin.csl 8dae693 rework style 070586b Localize IEEE dates (#5835) 588fbfe Hopefully fix sorting in CSE author-date (#5834) 1291d72 Update physiotherapy-theory-and-practice.csl 1a64076 New Style for "Neue Kriminalpolitik (Deutsch)" (#5586) eac93a0 Create physiotherapy-theory-and-practice.csl 2e7a9f6 Merge pull request #5832 from POBrien333/patch-996 4dfad5f Update administrative-science-quarterly.csl f56db0f Merge pull request #5829 from benthamite/patch-1 526b0b3 Update effective-altruism-wiki.csl e5b11eb Update effective-altruism-wiki.csl f2a7301 Update effective-altruism-wiki.csl 5b166c2 Update university-of-roehampton-harvard.csl (#5831) b231514 Update european-society-of-cardiology.csl (#5519) dbd902c Re-indent CSL styles fe8892b Update effective-altruism-wiki.csl 734fa7d Update effective-altruism-wiki.csl 2430a32 Create effective-altruism-wiki.csl a5101b6 update modern-pathology.csl (#5828) 16f77c8 Update tyndale-bulletin.csl ea8804e Create the-korean-journal-of-mycology.csl (#5822) 22d8be0 Patch 3 (#5825) 0220ba2 Create jcom.csl (#5819) cd7b72b Create taylor-and-francis-council-of-science-editors-numeric.csl (#5824) 0033383 Create gomis-senegalese-medicine-thesis.csl (#5774) b221bcc Create mycobiology.csl (#5821) d7056f9 Re-indent CSL styles 04ae08b Update tyndale-bulletin.csl 24bd577 Re-indent CSL styles e08d431 clean up logging f6ac2a6 Update tyndale-bulletin.csl e4c07f8 this should be the PR repo name 7f9936c fetch pull request from originating repo f232f51 Update tyndale-bulletin.csl e2ae4fc Update tyndale-bulletin.csl f09f8db Update Sheldon to csl-styles 2.0 a4d7dec re-indent style a903664 sheldon 7db75fc sheldon f08dc96 sheldon 35b8bfa sheldon 04e28ca pull req checkout 9c9d061 pull req checkout 9249490 sheldon reindent 9792be6 sheldon reindent 3bced06 reindent & commit 7cc9735 Update styles README and GitHub Actions with 1.0.2 information (#5803) 205c4ea remaining dependents 9458056 fix broken styles 07ba7af fix dependents e2e7842 Fix term hacks for transition to CSL 1.0.2 (#5801) 5c8c5e9 Replace "sub verbo" with "sub-verbo" (#5799) 7ec637c Fix CSL styles bcd3054 update to csl/citeproc-ruby 2.0 ec13830 bundle update git-subtree-dir: buildres/csl/csl-styles git-subtree-split: 5563ccc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A first approach to convert the add/edit group dialog to the mvvm-pattern and a step forwards to implement the changes @MartinKarim supposed in #4684 .
Still some problems and things to talk about.
The most obvious one is: GroupDescription. I really don't know what to do with this. Personally I think, this description stuff shuld go into the help page. It's a wild mesh-up of error messages, validation messages for the regex and some hints about the group...
The next thing are the different group types. @MartinKarim supposed to merge most of them, as they mostly intersect each other. But if we do this, we need to provide migrations for the old groups too. This would probably go far beyond this PR.
Screenshot below
The old labels of the checkboxes a displayed as a tooltip now. But they should be better placed in the help page.
I have some problems with the validators. Somehow if i bind the disableProperty of the ok button to validProperty of the validator, Java throws a NPE. If I don't get this working, I need to think about a workaround.
I need to stop procrastinating now and get back to studying.