Skip to content

Observable preferences F (GuiPreferences, Proxy and Remote)#8166

Merged
Siedlerchr merged 5 commits into
mainfrom
guiPreferences
Oct 22, 2021
Merged

Observable preferences F (GuiPreferences, Proxy and Remote)#8166
Siedlerchr merged 5 commits into
mainfrom
guiPreferences

Conversation

@calixtus

@calixtus calixtus commented Oct 19, 2021

Copy link
Copy Markdown
Member

Follow up to #8165

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@calixtus calixtus added component: preferences status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers dev: code-quality Issues related to code or architecture decisions labels Oct 19, 2021
# Conflicts:
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
@calixtus calixtus changed the title Observable preferences F (GuiPreferences) Observable preferences F (GuiPreferences, Proxy and Remote) Oct 20, 2021
storeRemoteSettings();
storeProxySettings();

storeProxySettings(new ProxyPreferences(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's late so I might missing something obvious here, but: You create a new ProxyPreference object here and storeProxySettings only calls the set* methods. Since there are no bindings of that proxypreference object to JabRefPreferences, this will essentially do nothing?!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're missing something. I think.
storeProxyPreferences takes just the members one by one of the newly created object and stores them in the original preferences object, that was supplied by JabRefPreferences. See NetworkTabViewModel::storeProxySettings:

    private void storeProxySettings(ProxyPreferences newProxyPreferences) {
        if (!newProxyPreferences.equals(proxyPreferences)) {
            ProxyRegisterer.register(newProxyPreferences);
        }

        proxyPreferences.setUseProxy(newProxyPreferences.shouldUseProxy());
        proxyPreferences.setHostname(newProxyPreferences.getHostname());
        proxyPreferences.setPort(newProxyPreferences.getPort());
        proxyPreferences.setUseAuthentication(newProxyPreferences.shouldUseAuthentication());
        proxyPreferences.setUsername(newProxyPreferences.getUsername());
        proxyPreferences.setPassword(newProxyPreferences.getPassword());
    }

The reason for creating a new preferences object here is to keep the test proxy button.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay thanks, I missed that indeed.

@Siedlerchr

Copy link
Copy Markdown
Member

l10n fails:

>Remote server port
  
  1. CHECK IF THE KEY IS REALLY NOT USED ANYMORE
  2. REMOVE THESE FROM THE ENGLISH LANGUAGE FILE
   ==> expected: <[]> but was: <[Remote server port]>

@calixtus

Copy link
Copy Markdown
Member Author

Thanks, just started IntelliJ a second ago to fix it, but you were quicker. :-)

@Siedlerchr Siedlerchr merged commit ae82589 into main Oct 22, 2021
@Siedlerchr Siedlerchr deleted the guiPreferences branch October 22, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: preferences dev: code-quality Issues related to code or architecture decisions 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.

3 participants