Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversion of preferencesDialog/advancedTab, networkTab and groupsTab to mvvm #5141

Merged
merged 12 commits into from
Aug 18, 2019

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Jul 20, 2019

follow-up to #5033

The next tab, this time advancedTab and networkTab combined
Also conversion of the groupsTab and some minor refactoring

advancedtab

groupstab


  • 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?)

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.

Looks good! Like the idea of the password caret

@calixtus calixtus mentioned this pull request Jul 20, 2019
24 tasks
@calixtus calixtus changed the title Conversion of preferencesDialog/advancedTab and networkTab to mvvm Conversion of preferencesDialog/advancedTab, networkTab and groupsTab to mvvm Jul 21, 2019
@calixtus
Copy link
Member Author

Sorry for this large PR. Besides the conversion/merge of the three tabs to mvvm, mainly small corrections and cosmetics I overlooked earlier.
Should be ready to review now

Network=Network
Please\ specify\ both\ hostname\ and\ port=Please specify both hostname and port
Please\ specify\ both\ username\ and\ password=Please specify both username and password
Search\ IEEEXplore=Search IEEEXplore
Copy link
Member

Choose a reason for hiding this comment

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

Normally it should be sufficient to have the Search %0. It would be interesting to see if this works in fxml somewhow.

Copy link
Member Author

@calixtus calixtus Jul 23, 2019

Choose a reason for hiding this comment

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

I tried around a little bit, but I could not find a real solution. The problem is in mixing Ressource-Resolution and dynamic binding in one property. The other option would be to put setText(Localization.lang("Search %0") + "IEEExplore") into the Java-View.
One thing i principally thought of: The term "Search" can have multiple translations. In this case, its a header. "Search IEEExplore" would probably translated into german into "Suche in IEEExplore". If we use "Search %0" here und want to use it e.g. for a search in the entries, you probably say in german "Suche nach %0"

@calixtus
Copy link
Member Author

I made an abstract-class to stop repeating the the getBuilder-stuff on every tab and reworded the abbreviated PrefsTab-Interface, so this is why this PR grew again.

@calixtus
Copy link
Member Author

calixtus commented Aug 9, 2019

Huh, travis failed because some github service was not available. However, works fine on my machine. I would really love to get some reply on my thoughts two weeks ago and how I should proceed...
And if there is no other issue, its ready to review...

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 9, 2019
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.

llgtm

@Siedlerchr
Copy link
Member

Sorry, I was away the whole last week,but codewise looks good to me. Was there anything else you wanted to know?

@calixtus
Copy link
Member Author

calixtus commented Aug 9, 2019

No panic 😄, I just wanted to know, if I should change the "Search IEEExplore" localization back to "Search %0"

@Siedlerchr
Copy link
Member

No, you can leave it, I tried to find a solution but seems there is not one for the fxml stuff.

@calixtus
Copy link
Member Author

calixtus commented Aug 9, 2019

ok, I'll leave it than that way.

@Siedlerchr
Copy link
Member

Looks good, can this be merged now?

@calixtus
Copy link
Member Author

sure

@Siedlerchr Siedlerchr merged commit f51ba49 into JabRef:master Aug 18, 2019
Siedlerchr added a commit to Siedlerchr/jabref that referenced this pull request Aug 23, 2019
* upstream/master:
  Bugfix/5045 : Modified the existing logic to comply crossref resolution with biblatex specification (JabRef#5086)
  Fix issue with missing year value in year column (JabRef#5197)
  Bump com.gradle.build-scan from 2.4 to 2.4.1 (JabRef#5199)
  Add citation commands to TexParser: autocite, blockcquote, and textcquote (JabRef#5195)
  Conversion of preferencesDialog/advancedTab, networkTab and groupsTab to mvvm (JabRef#5141)
  Fix Permissions of LaTeXintegration (JabRef#5134)
  Border for group color indicator and some space for tooltip (JabRef#5190)
  Fix issue 5152, tooltip and icon added to group cell (JabRef#5191)
  Fix tooltips in CitationsDisplay (JabRef#5188)
@calixtus calixtus deleted the preferences_advanced_mvvm branch August 24, 2019 10:13
# for free to join this conversation on GitHub. Already have an account? # 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.

2 participants