From 3b3ef02a5861fa044d4a8b61d9f00c67994d5095 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20jaen=20Ma=C5=84ko?= Date: Wed, 10 Jul 2024 16:18:08 +0200 Subject: [PATCH] IJPL-13931 Ensure settings categories are respected in the initial sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are two parts to the fix: * explicitly flushing settings to disk before the initial sync — while the sync subsystem correctly filters out files not matching the selected sync categories, it only seems to take into account files that are actually persisted on disk when uploading them. When setting up sync, the settings object initially remains in-memory (at least until the time IDE decides to flush it on it's own), so initial upload might be missing the settings state, giving appearance of all the categories being synced, instead of just the selected ones, * correctly categorise plugin settings — `SettingsSyncFiltering.getSubCategory` was written to only classify font settings properly, so you could not disable a specific plugin from syncing. This is now fixed by using plugin ID for category while filtering. It might make sense to unify this code with `SyncPluginGroup` one way or another, but it was simpler to just duplicate the behaviour for this fix. Signed-off-by: Sergey Pak GitOrigin-RevId: 88ab02f1198479eaad8966b7890748b87e53f5e0 --- .../settingsSync/git/record/ChangeRecord.kt | 2 +- .../settingsSync/SettingsSyncBridge.kt | 3 ++ .../settingsSync/SettingsSyncFiltering.kt | 44 +++++++++---------- .../settingsSync/config/SyncPluginsGroup.kt | 1 + 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt b/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt index f58a606e14c8..38c048ef83d7 100644 --- a/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt +++ b/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt @@ -129,7 +129,7 @@ internal class ChangeRecord(commitId: Int, if (fileName == GitSettingsLog.PLUGINS_FILE) return SettingsCategory.PLUGINS //workaround empty category - return getCategory(fileName) ?: SettingsCategory.OTHER + return getCategory(fileName)?.first ?: SettingsCategory.OTHER } val changesCategories = changes.map { getChangeCategory(it) }.distinct().sortedBy { getCategoryOrder(it) }.map { toString(it) } diff --git a/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncBridge.kt b/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncBridge.kt index a154d149d59d..a14eec4e7f80 100644 --- a/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncBridge.kt +++ b/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncBridge.kt @@ -92,6 +92,9 @@ class SettingsSyncBridge( coroutineScope.launch { withProgressText(SettingsSyncBundle.message(initMode.messageKey)) { try { + // Always explicitly flush settings – if this is not done before sending sync events, then remotely synced settings + // might not contain the most up–to–date settings state (e.g. sync settings will be stale). + saveIdeSettings() settingsLog.initialize() // the queue is not activated initially => events will be collected but not processed until we perform all initialization tasks diff --git a/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt b/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt index fa83bc6a90fc..82c7606cac9b 100644 --- a/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt +++ b/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt @@ -10,20 +10,19 @@ import com.intellij.openapi.util.text.StringUtil import com.intellij.serviceContainer.ComponentManagerImpl import com.intellij.settingsSync.config.EDITOR_FONT_SUBCATEGORY_ID import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.ConcurrentMap internal fun isSyncCategoryEnabled(fileSpec: String): Boolean { val rawFileSpec = removeOsPrefix(fileSpec) if (rawFileSpec == SettingsSyncSettings.FILE_SPEC) return true - val category = getSchemeCategory(rawFileSpec) ?: getCategory(rawFileSpec) ?: return false + val (category, subCategory) = getSchemeCategory(rawFileSpec) ?: getCategory(rawFileSpec) ?: return false if (category != SettingsCategory.OTHER && SettingsSyncSettings.getInstance().isCategoryEnabled(category)) { - val subCategory = getSubCategory(fileSpec) if (subCategory != null) { return SettingsSyncSettings.getInstance().isSubcategoryEnabled(category, subCategory) } + return true } return false @@ -34,26 +33,22 @@ private fun removeOsPrefix(fileSpec: String): String { return if (fileSpec.startsWith(osPrefix)) StringUtil.trimStart(fileSpec, osPrefix) else fileSpec } -private fun getCategory(componentClasses: List>>): SettingsCategory { - when { - componentClasses.isEmpty() -> return SettingsCategory.OTHER - componentClasses.size == 1 -> return ComponentCategorizer.getCategory(componentClasses[0]) - else -> { - componentClasses.forEach { - val category = ComponentCategorizer.getCategory(it) - if (category != SettingsCategory.OTHER) { - // Once found, ignore any other possibly conflicting definitions - return category - } - } - return SettingsCategory.OTHER +private fun getCategory(fileName: String, componentClasses: List>>): Pair { + componentClasses.forEach { + val category = ComponentCategorizer.getCategory(it) + + if (category != SettingsCategory.OTHER) { + // Once found, ignore any other possibly conflicting definitions + return (category to getSubCategory(fileName)) } } + + return SettingsCategory.OTHER to null } -private val categoryCache: ConcurrentMap = ConcurrentHashMap() +private val categoryCache: ConcurrentHashMap> = ConcurrentHashMap() -fun getCategory(fileName: String): SettingsCategory? { +fun getCategory(fileName: String): Pair? { categoryCache[fileName]?.let { cachedCategory -> return cachedCategory } @@ -64,14 +59,14 @@ fun getCategory(fileName: String): SettingsCategory? { return null } - val category = getSchemeCategory(fileName) ?: getCategory(componentClasses) + val category = getSchemeCategory(fileName) ?: getCategory(fileName, componentClasses) categoryCache[fileName] = category return category } -private fun getSchemeCategory(fileSpec: String): SettingsCategory? { +private fun getSchemeCategory(fileSpec: String): Pair? { // fileSpec is e.g. keymaps/mykeymap.xml val separatorIndex = fileSpec.indexOf("/") val directoryName = if (separatorIndex >= 0) fileSpec.substring(0, separatorIndex) else fileSpec // e.g. 'keymaps' @@ -82,12 +77,13 @@ private fun getSchemeCategory(fileSpec: String): SettingsCategory? { settingsCategory = it.getSettingsCategory() } } - return settingsCategory + if (settingsCategory == null) { + return null + } + + return settingsCategory!! to null } -fun getFileSpec(path: String): String { - return removeOsPrefix(path) -} private fun getSubCategory(fileSpec: String): String? { if (fileSpec == AppEditorFontOptions.STORAGE_NAME) diff --git a/plugins/settings-sync/src/com/intellij/settingsSync/config/SyncPluginsGroup.kt b/plugins/settings-sync/src/com/intellij/settingsSync/config/SyncPluginsGroup.kt index d1f51c023cb8..ec71b05f4511 100644 --- a/plugins/settings-sync/src/com/intellij/settingsSync/config/SyncPluginsGroup.kt +++ b/plugins/settings-sync/src/com/intellij/settingsSync/config/SyncPluginsGroup.kt @@ -18,6 +18,7 @@ internal class SyncPluginsGroup : SyncSubcategoryGroup { PluginManagerCore.plugins.forEach { if (!it.isBundled && SettingsSyncPluginCategoryFinder.getPluginCategory(it) == SettingsCategory.PLUGINS) { bundledPluginsDescriptor.isSubGroupEnd = true + // NOTE: the code in `com.intellij.settingsSync.SettingsSyncFilteringKt.getSubCategory` relies on the value being plugin ID descriptors.add(getOrCreateDescriptor(it.name, it.pluginId.idString)) } }