Fix for #4628 - cant save new content selectors #4658

ricantech · 2019-02-17T11:39:08Z

Fixes #4628
Dialog converted into JavaFX

old functionalities are preserved
new validations added:
** error message thrown when dialog invoked but not active connection exists
** remove keyword button is enabled just in case some keyword is selected

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2019-02-17T13:36:14Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogView.java

+    }
+
+    private void initListView(ListView<String> listViewToInit, Supplier<ObservableList<String>> backingList, ChangeListener<String> onSelectedListener) {
+        listViewToInit.setItems(backingList.get());


Better use bindings, so you can get a better separation between the view and the ViewModel.

myListView.itemsProperty().bind(viewModel.backingListProperty);
Same also applies to selectedItemProperty
Sth like this viewModel.seletedItemProperty().bind(listView.getSelectionModel().selectedItemProperty());

This also removes the need to have the ChangeListener, because the bindings under the hood have the change listener implemented

Fixed 205170c

Siedlerchr · 2019-02-17T13:36:33Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogView.java

+    }
+
+    private Optional<String> getSelectedFieldName() {
+        return Optional.of(fieldNamesListView.getSelectionModel()).map(SelectionModel::getSelectedItem);


This should also be moved to the viewModel

And if you change this to a BoooleanProperty in the viewModel, you can directly bind to it:
removeFieldNameButton.disableProperty().bind(viewModel.isSelectedFieldNamePrsent))

public BooleanProperty isSelectedFieldNamePresent{ return ! selectedItemProperty().getValue().isEmpty() }

Fixed 205170c

Siedlerchr · 2019-02-17T13:40:37Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogViewModel.java

+        );
+    }
+
+    ObservableList<String> getFieldNamesBackingList() {


Convert this to a ListProperty fieldNamesProperty()

Fixed e4f3ea3

Siedlerchr · 2019-02-17T13:41:20Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogViewModel.java

+    private final BasePanel basePanel;
+    private final Map<String, List<String>> fieldNameKeywordsMap = new HashMap<>();
+
+    private ObservableList<String> fieldNames = FXCollections.observableArrayList();


Convert this to a

ListProperty<String> fieldNamesProperty = new SimpleListProperty(FXCollections.observableArrayList())

Fixed e4f3ea3

Siedlerchr · 2019-02-17T13:44:54Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogViewModel.java

+    }
+
+    ObservableList<String> getKeywordsBackingList() {
+        return keywords;


same as above

Fixed e4f3ea3

Siedlerchr

Thanks for your contribution!
There are some things regarding the javafx property and binding handling which are confusing at first, but actually help to simplify complex code.

Siedlerchr · 2019-02-17T13:48:37Z

Additionally, it would be nice if you could add a screenshot of the converted dialog

ricantech · 2019-02-17T20:13:31Z

@Siedlerchr Hi, thanks for the review.

I hope I was able to incorporate all the changes mentioned above - ListProperty + bindings
Here is the screenshot of converted dialog:

tobiasdiez

Thanks for your PR. The code looks good to me and I've only one very small remark. If this is fixed as well, we can merge.

tobiasdiez · 2019-02-17T21:59:27Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogView.java

+
+    private void initListView(ListView<String> listViewToInit, Supplier<ListProperty<String>> backingList) {
+        listViewToInit.itemsProperty().bind(backingList.get());
+        listViewToInit.getSelectionModel().select(FIRST_ELEMENT);


I think there already is a selectFirst method in the selection model.

Fixed - thanks for the hint c00274c

ricantech · 2019-02-18T06:27:12Z

@tobiasdiez Thanks for the review - changes have been made

…- review changes

Siedlerchr · 2019-02-18T07:39:11Z

src/main/java/org/jabref/gui/contentselector/ContentSelectorDialogView.java

+        this.getDialogPane().setPrefSize(375, 475);
+
+        this.basePanel = jabRefFrame.getCurrentBasePanel();
+        this.dialogService = jabRefFrame.getDialogService();


I would change the constructor to only accept basePanel as Argument, because you can do the following:
@Inject private DialogService dialogService;
This automatically injects (passes an instance of it) to the class

Just this one change and it looks good to me ;)

Good point :) fixed - c32fe6a

tobiasdiez · 2019-02-18T21:11:01Z

Thanks again for your contribution! Looking forward to your next PR ;-)

ricantech · 2019-02-18T21:16:48Z

Thanks guys for super guidance!

Siedlerchr · 2019-02-19T07:29:30Z

@ricantech We have to thank you for your contribution! We hope you are interested in fixing some other issues as well!

* upstream/master: Bump mockito-core from 2.24.0 to 2.24.5 (#4663) Fix for #4628 - cant save new content selectors (#4658) Fix issues with double spaces. (#4662)

…tofx * upstream/master: fix copy base bug in Strings Dialog validation Bump mockito-core from 2.24.0 to 2.24.5 (#4663) Fix for #4628 - cant save new content selectors (#4658) Fix issues with double spaces. (#4662)

8695d97 Update societe-francaise-detude-de-la-ceramique-antique-en-gaule.csl (JabRef#4660) 0dc2e37 Update freshwater-science.csl (JabRef#4658) 118cfab Update ayer.csl (JabRef#4659) 3ba11bc Create optics-letters.csl (JabRef#4661) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 8695d97

900e4d5 Create revista-de-biologia-marina-y-oceanografia.csl (#4671) 01672e3 polimdosains-politeknik-negeri-manado.csl (#4662) d97d4b7 Update iso690-author-date-sk.csl (#4669) 22e0bab Create trabajos-de-prehistoria.csl (#4670) 6c6bf37 Create proceedings-of-the-joint-international-grassland-and-international-rangeland-congress-2021.csl (#4667) b91deaf Create rhinology.csl (#4666) d3b58b2 Add OSA dependents (#4668) a0d4bab Create the-international-journal-of-tuberculosis-and-lung-disease.csl (#4663) 5a865de Update harvard-university-for-the-creative-arts.csl (#4665) 8695d97 Update societe-francaise-detude-de-la-ceramique-antique-en-gaule.csl (#4660) 0dc2e37 Update freshwater-science.csl (#4658) 118cfab Update ayer.csl (#4659) 3ba11bc Create optics-letters.csl (#4661) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 900e4d5

c1793d2 Create isnad-metinici.csl (JabRef#4704) 099f4a1 Create isnad-dipnotlu.csl (JabRef#4703) 8e8639e Update norois.csl (JabRef#4706) 62f6221 Create amsterdam-university-press.csl (JabRef#4699) c31d9ca Create tagungsberichte-der-historischen-kommission-fur-ost-und-westpr… (JabRef#4700) 34220d3 Change apa-fr-provost.csl to fr-CA locale (JabRef#4701) bbdc9c4 Update to better match RMIT Easy Cite (JabRef#4697) 7c506d9 Create Anatomia Histologia Embryologia csl file (JabRef#4688) 61f3dc5 Update pravnik.csl, masarykova-univerzita-pravnicka-fakulta.csl and iso690-full-note-cs.csl (JabRef#4705) 449eaec add DOI to mdpi.csl back (JabRef#4695) 642002e Create muni-law.csl (JabRef#4691) 483d04a Create iso690-author-date-pt (JabRef#4694) 505c006 Update pravnik.csl (JabRef#4690) eb260ef Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4692) 9279f0d Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4693) 70f7be6 Create iso690-full-note-cs-pravnik.csl (JabRef#4686) db54e56 Update CONTRIBUTING.md 2f16a9b Update CONTRIBUTING.md 4cd05ba Create associacao-brasileira-de-normas-tecnicas-ufrgs-footnote-ibid.csl (JabRef#4369) 4d4655e Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4684) 5948265 Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4683) f8b744c Create aberdeen-university-school-of-education-harvard.csl (JabRef#4685) 807930a Update trabajos-de-prehistoria.csl (JabRef#4680) c25709f Update society-for-american-archaeology.csl (JabRef#4681) 86f888a Update archeosciences.csl (JabRef#4682) 99c4fd3 Create sylwan.csl (JabRef#4678) d9ba2de Update equine-veterinary-education.csl (JabRef#4677) 7d30659 Update equine-veterinary-education.csl (JabRef#4675) 9a7791f Update equine-veterinary-education.csl (JabRef#4674) 0a46235 Create norois.csl (JabRef#4673) 88726cb Update Rhinology (JabRef#4672) 900e4d5 Create revista-de-biologia-marina-y-oceanografia.csl (JabRef#4671) 01672e3 polimdosains-politeknik-negeri-manado.csl (JabRef#4662) d97d4b7 Update iso690-author-date-sk.csl (JabRef#4669) 22e0bab Create trabajos-de-prehistoria.csl (JabRef#4670) 6c6bf37 Create proceedings-of-the-joint-international-grassland-and-international-rangeland-congress-2021.csl (JabRef#4667) b91deaf Create rhinology.csl (JabRef#4666) d3b58b2 Add OSA dependents (JabRef#4668) a0d4bab Create the-international-journal-of-tuberculosis-and-lung-disease.csl (JabRef#4663) 5a865de Update harvard-university-for-the-creative-arts.csl (JabRef#4665) 8695d97 Update societe-francaise-detude-de-la-ceramique-antique-en-gaule.csl (JabRef#4660) 0dc2e37 Update freshwater-science.csl (JabRef#4658) 118cfab Update ayer.csl (JabRef#4659) 3ba11bc Create optics-letters.csl (JabRef#4661) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: c1793d2

06785fa Update le-tapuscrit-author-date.csl (JabRef#4708) ada5282 Update norois.csl (JabRef#4707) c1793d2 Create isnad-metinici.csl (JabRef#4704) 099f4a1 Create isnad-dipnotlu.csl (JabRef#4703) 8e8639e Update norois.csl (JabRef#4706) 62f6221 Create amsterdam-university-press.csl (JabRef#4699) c31d9ca Create tagungsberichte-der-historischen-kommission-fur-ost-und-westpr… (JabRef#4700) 34220d3 Change apa-fr-provost.csl to fr-CA locale (JabRef#4701) bbdc9c4 Update to better match RMIT Easy Cite (JabRef#4697) 7c506d9 Create Anatomia Histologia Embryologia csl file (JabRef#4688) 61f3dc5 Update pravnik.csl, masarykova-univerzita-pravnicka-fakulta.csl and iso690-full-note-cs.csl (JabRef#4705) 449eaec add DOI to mdpi.csl back (JabRef#4695) 642002e Create muni-law.csl (JabRef#4691) 483d04a Create iso690-author-date-pt (JabRef#4694) 505c006 Update pravnik.csl (JabRef#4690) eb260ef Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4692) 9279f0d Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4693) 70f7be6 Create iso690-full-note-cs-pravnik.csl (JabRef#4686) db54e56 Update CONTRIBUTING.md 2f16a9b Update CONTRIBUTING.md 4cd05ba Create associacao-brasileira-de-normas-tecnicas-ufrgs-footnote-ibid.csl (JabRef#4369) 4d4655e Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4684) 5948265 Update instituto-brasileiro-de-informacao-em-ciencia-e-tecnologia-abn… (JabRef#4683) f8b744c Create aberdeen-university-school-of-education-harvard.csl (JabRef#4685) 807930a Update trabajos-de-prehistoria.csl (JabRef#4680) c25709f Update society-for-american-archaeology.csl (JabRef#4681) 86f888a Update archeosciences.csl (JabRef#4682) 99c4fd3 Create sylwan.csl (JabRef#4678) d9ba2de Update equine-veterinary-education.csl (JabRef#4677) 7d30659 Update equine-veterinary-education.csl (JabRef#4675) 9a7791f Update equine-veterinary-education.csl (JabRef#4674) 0a46235 Create norois.csl (JabRef#4673) 88726cb Update Rhinology (JabRef#4672) 900e4d5 Create revista-de-biologia-marina-y-oceanografia.csl (JabRef#4671) 01672e3 polimdosains-politeknik-negeri-manado.csl (JabRef#4662) d97d4b7 Update iso690-author-date-sk.csl (JabRef#4669) 22e0bab Create trabajos-de-prehistoria.csl (JabRef#4670) 6c6bf37 Create proceedings-of-the-joint-international-grassland-and-international-rangeland-congress-2021.csl (JabRef#4667) b91deaf Create rhinology.csl (JabRef#4666) d3b58b2 Add OSA dependents (JabRef#4668) a0d4bab Create the-international-journal-of-tuberculosis-and-lung-disease.csl (JabRef#4663) 5a865de Update harvard-university-for-the-creative-arts.csl (JabRef#4665) 8695d97 Update societe-francaise-detude-de-la-ceramique-antique-en-gaule.csl (JabRef#4660) 0dc2e37 Update freshwater-science.csl (JabRef#4658) 118cfab Update ayer.csl (JabRef#4659) 3ba11bc Create optics-letters.csl (JabRef#4661) a51eb4f Create societe-francaise-detude-de-la-ceramique-antique-en-gaule.csl (JabRef#4653) 9032e5b Create geografia-fisica-e-dinamica-quaternaria.csl (JabRef#4650) 38996d6 Create multimed.csl (JabRef#4644) 9cd28d3 Update journal-of-urban-technology.csl (JabRef#4652) 265c93f Correct Acta Palaeontologica Polonica in-text citation format (JabRef#4655) 9cf9ddc Create bioscience.csl (JabRef#4654) 277f340 Update brazilian-journal-of-veterinary-research-and-animal-science.csl (JabRef#4656) a3e4ecf Update associacao-brasileira-de-normas-tecnicas-ufrgs-initials.csl (JabRef#4461) 960b316 Create journal-of-urban-technology.csl (JabRef#4645) ddf2572 Create universitat-stuttgart-mpp-standard.csl (JabRef#4639) bde92d6 Update environmental-microbiology.csl (JabRef#4641) efa68bc Update presses-universitaires-de-rennes.csl (JabRef#4640) 0872e3e Fixes JabRef#4648 (JabRef#4649) 9a159a4 Create studi-e-materiali-di-storia-delle-religioni.csl (JabRef#4642) e63f13e further updates for websites and paper-conference Update ieee.csl (JabRef#4646) 0cb6762 Create Bulletin du Centre d’études médiévales d’Auxerre (JabRef#4635) 784a1c9 Update instituto-de-pesquisas-energeticas-e-nucleares.csl (JabRef#4634) e5f9eed Update ieee.csl (JabRef#4636) e82a783 Fix paper-conference in journal-of-new-zealand-grasslands.csl (JabRef#4637) 7365ce8 fix editor label for french apa.csl (JabRef#4616) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 06785fa

Siedlerchr reviewed Feb 17, 2019

View reviewed changes

Siedlerchr requested changes Feb 17, 2019

View reviewed changes

tobiasdiez requested changes Feb 17, 2019

View reviewed changes

ricantech added 11 commits February 18, 2019 07:46

Design of content selector dialog + initial view commit

79367fa

Restyle dialog design + add basic functionalities for field names

27e44b1

Add logic regarding keywords

f3d18cf

Minor rework of keywords logic + toggling of delete button

7b2a2dc

Unify naming convention

dd036eb

Logic tuneup

98b423e

Add english translations

bd0a49e

Add translations + imports

cca70cc

Convert ObservableList into ListProperty - review changes

f60ed8c

Rework actions on bindings instead of using directly change listener …

7d2e62a

…- review changes

Replace setting of first element with built-in method in selection model

480487c

Siedlerchr reviewed Feb 18, 2019

View reviewed changes

Siedlerchr mentioned this pull request Feb 18, 2019

Convert/rework rest of the Swing Dialogs to javafx #3861

Closed

36 tasks

Use Inject for DialogService

c32fe6a

tobiasdiez merged commit 7e1bf18 into JabRef:master Feb 18, 2019

Siedlerchr added a commit that referenced this pull request Feb 19, 2019

Merge remote-tracking branch 'upstream/master'

1b6c7b5

* upstream/master: Bump mockito-core from 2.24.0 to 2.24.5 (#4663) Fix for #4628 - cant save new content selectors (#4658) Fix issues with double spaces. (#4662)

luizphilipe2 mentioned this pull request Jun 26, 2021

Convert AutoLinkFilesAction to JavafX luizphilipe2/jabref_experiment#32

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #4628 - cant save new content selectors #4658

Fix for #4628 - cant save new content selectors #4658

ricantech commented Feb 17, 2019 •

edited by Siedlerchr

Siedlerchr Feb 17, 2019

ricantech Feb 17, 2019

Siedlerchr Feb 17, 2019

Siedlerchr Feb 17, 2019 •

edited

ricantech Feb 17, 2019

Siedlerchr Feb 17, 2019

ricantech Feb 17, 2019

Siedlerchr Feb 17, 2019 •

edited

ricantech Feb 17, 2019

Siedlerchr Feb 17, 2019

ricantech Feb 17, 2019

Siedlerchr left a comment

Siedlerchr commented Feb 17, 2019

ricantech commented Feb 17, 2019

tobiasdiez left a comment

tobiasdiez Feb 17, 2019

ricantech Feb 18, 2019 •

edited

ricantech commented Feb 18, 2019

Siedlerchr Feb 18, 2019

Siedlerchr Feb 18, 2019

ricantech Feb 18, 2019

tobiasdiez commented Feb 18, 2019

ricantech commented Feb 18, 2019

Siedlerchr commented Feb 19, 2019

Fix for #4628 - cant save new content selectors #4658

Fix for #4628 - cant save new content selectors #4658

Conversation

ricantech commented Feb 17, 2019 • edited by Siedlerchr

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Feb 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Feb 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented Feb 17, 2019

ricantech commented Feb 17, 2019

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricantech Feb 18, 2019 • edited

Choose a reason for hiding this comment

ricantech commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Feb 18, 2019

ricantech commented Feb 18, 2019

Siedlerchr commented Feb 19, 2019

ricantech commented Feb 17, 2019 •

edited by Siedlerchr

Siedlerchr Feb 17, 2019 •

edited

Siedlerchr Feb 17, 2019 •

edited

ricantech Feb 18, 2019 •

edited