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
Desktop: Resolves #8625: Show missing sync password warning and link to FAQ #8644
Changes from 2 commits
72d364a
b5605a9
480dbfb
4aba7b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,14 +12,17 @@ const { connect } = require('react-redux'); | |
const { themeStyle } = require('@joplin/lib/theme'); | ||
const pathUtils = require('@joplin/lib/path-utils'); | ||
import SyncTargetRegistry from '@joplin/lib/SyncTargetRegistry'; | ||
const shared = require('@joplin/lib/components/shared/config-shared.js'); | ||
const shared = require('@joplin/lib/components/shared/config/config-shared.js'); | ||
import ClipperConfigScreen from '../ClipperConfigScreen'; | ||
import restart from '../../services/restart'; | ||
import PluginService from '@joplin/lib/services/plugins/PluginService'; | ||
import { getDefaultPluginsInstallState, updateDefaultPluginsInstallState } from '@joplin/lib/services/plugins/defaultPlugins/defaultPluginsUtils'; | ||
import getDefaultPluginsInfo from '@joplin/lib/services/plugins/defaultPlugins/desktopDefaultPluginsInfo'; | ||
import JoplinCloudConfigScreen from '../JoplinCloudConfigScreen'; | ||
import ToggleAdvancedSettingsButton from './controls/ToggleAdvancedSettingsButton'; | ||
import shouldShowMissingPasswordWarning from '@joplin/lib/components/shared/config/shouldShowMissingPasswordWarning'; | ||
import shim from '@joplin/lib/shim'; | ||
import StyledLink from '../style/StyledLink'; | ||
const { KeymapConfigScreen } = require('../KeymapConfig/KeymapConfigScreen'); | ||
|
||
const settingKeyToControl: any = { | ||
|
@@ -181,6 +184,35 @@ class ConfigScreenComponent extends React.Component<any, any> { | |
if (section.name === 'sync') { | ||
const syncTargetMd = SyncTargetRegistry.idToMetadata(settings['sync.target']); | ||
const statusStyle = { ...theme.textStyle, marginTop: 10 }; | ||
const warningStyle = { ...theme.textStyle, color: theme.colorWarn }; | ||
|
||
// Don't show the missing password warning if the user just changed the sync target (but hasn't | ||
// saved yet). | ||
const matchesSavedTarget = settings['sync.target'] === this.props.settings['sync.target']; | ||
if (matchesSavedTarget && shouldShowMissingPasswordWarning(settings['sync.target'], settings)) { | ||
const openMissingPasswordFAQ = () => | ||
bridge().openExternal('https://joplinapp.org/faq#why-did-my-sync-and-encryption-passwords-disappear-after-updating-joplin'); | ||
|
||
const macInfoLink = ( | ||
<StyledLink href="#" | ||
onClick={openMissingPasswordFAQ} | ||
style={theme.linkStyle} | ||
> | ||
{_('Why is my password missing?')} | ||
</StyledLink> | ||
); | ||
|
||
// The FAQ section related to missing passwords is specific to MacOS/ARM -- only show it | ||
// in that case. | ||
const showMacInfoLink = shim.isMac(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may make sense to update this condition such that the warning is only shown on ARM 64 Macs. I only have access to an x86_64 Mac, however, so would be unable to test an ARM64-specific condition manually. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes please enable it only for ARM64. I think you just need to check |
||
|
||
settingComps.push( | ||
<p key='missing-password-warning' style={warningStyle}> | ||
{_('Warning: Missing password.')} | ||
{showMacInfoLink ? macInfoLink : null} | ||
</p> | ||
); | ||
} | ||
|
||
if (syncTargetMd.supportsConfigCheck) { | ||
const messages = shared.checkSyncConfigMessages(this); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import SyncTargetRegistry from '../../../SyncTargetRegistry'; | ||
|
||
const shouldShowMissingPasswordWarning = (syncTargetId: number, settings: any) => { | ||
laurent22 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// List of sync targets that expect a non-empty password setting | ||
const targetsExpectingPassword = [ | ||
'webdav', 'nextcloud', 'amazon_s3', 'joplinServer', 'joplinCloud', | ||
].map(name => SyncTargetRegistry.nameToId(name)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually sync target properties go in the SyncTargetXxx.ts class so that it's easy to find them back all in the same place. In that case you'd create a new property in BaseSyncTarget, probably true by default since most sync targets require a password, and you'd override for those that don't. |
||
|
||
const expectsPassword = targetsExpectingPassword.includes(syncTargetId); | ||
return expectsPassword && settings[`sync.${syncTargetId}.password`] === ''; | ||
}; | ||
|
||
export default shouldShowMissingPasswordWarning; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible alternative:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just Help would be good. That's a temporary message that we'll keep just for the ARM64 transition, so we don't want to create too many new translatable strings for it