From fef3740fa51b5bce4ac10eae02856ef14ec6ebde Mon Sep 17 00:00:00 2001 From: Jon Robson Date: Wed, 29 May 2024 16:05:22 -0700 Subject: [PATCH] Clean up preferences code - Reference preview preferences should be defined inside Cite - Don't use constants for seldom used strings to make the code more readable. - The lightweght ext.popups module is now always sent to the user This was a micro-optimization and not necessary to do Bug: T365538 Depends-On: Ic479c0a381ee16d1abcecfdd5ee48f0afccc1d3f Change-Id: Ie8fa1672b9fdbb1c6b840dde5c9060a20a625adb --- extension.json | 9 +---- i18n/en.json | 6 +-- i18n/qqq.json | 6 +-- includes/PopupsContext.php | 45 +-------------------- includes/PopupsHooks.php | 63 ++--------------------------- tests/phpunit/PopupsContextTest.php | 44 +------------------- tests/phpunit/PopupsHooksTest.php | 57 ++++++++------------------ 7 files changed, 25 insertions(+), 205 deletions(-) diff --git a/extension.json b/extension.json index bfa4fa82f..63d8c299c 100644 --- a/extension.json +++ b/extension.json @@ -22,7 +22,6 @@ "BeforePageDisplay": "PopupsHooks", "ResourceLoaderGetConfigVars": "PopupsHooks", "GetPreferences": "PopupsHooks", - "UserGetDefaultOptions": "PopupsHooks", "MakeGlobalVariablesScript": "PopupsHooks" }, "HookHandlers": { @@ -58,10 +57,6 @@ "description": "@var bool: Whether the option to enable/disable Page Previews should be hidden on Preferences page. False by default", "value": false }, - "PopupsOptInDefaultState": { - "description": "@var string:['1'|'0'] Default Page Previews visibility for old accounts. Has to be a string as a compatibility with beta feature settings. For more info see @T191888", - "value": "1" - }, "PopupsConflictingNavPopupsGadgetName": { "description": "@var string: Navigation popups gadget name", "value": "Navigation_popups" @@ -179,9 +174,7 @@ "includes/ServiceWiring.php" ], "DefaultUserOptions": { - "popups": "1", - "popupsreferencepreviews": "0", - "popups-reference-previews": "0" + "popups": "1" }, "ConditionalUserOptions": { "popups": [ diff --git a/i18n/en.json b/i18n/en.json index f525b3cf4..4bf3198c7 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -22,9 +22,5 @@ "prefs-reading": "Reading preferences", "popups-prefs-optin": "Enable page previews (get quick previews of a topic while reading a page)", "popups-prefs-disable-nav-gadgets-info": "You have to [[$1|disable the Navigation popups gadget]] in your Gadgets preferences to enable page previews.", - "popups-prefs-conflicting-gadgets-info": "Certain gadgets and other customizations may affect the performance of this feature. If you experience problems please review your gadgets and user scripts, including global ones.", - "popups-prefs-navpopups-gadget-conflict-info": "You have the [[$1|Navigation popups]] gadget enabled, so you won't see previews provided by this feature. Depending on your wiki, the gadget may have a slightly different name. If you continue to experience issues, please review your gadgets and user scripts, including global ones.", - "popups-prefs-reftooltips-gadget-conflict-info": "You have the [[$1|Reference Tooltips]] gadget enabled, so you won't see reference previews but will still see page previews. Depending on your wiki, the gadget may have a slightly different name. If you continue to experience issues, please review your gadgets and user scripts, including global ones.", - "popups-prefs-reftooltips-and-navpopups-gadget-conflict-info": "You have the [[$1|Navigation popups]] and [[$1|Reference Tooltips]] gadgets enabled, so you won't see previews provided by this feature. Depending on your wiki, the gadgets may have slightly different names. If you continue to experience issues, please review your gadgets and user scripts, including global ones.", - "popups-refpreview-user-preference-label": "Enable reference previews (get quick previews of a reference while reading a page)" + "popups-prefs-conflicting-gadgets-info": "Certain gadgets and other customizations may affect the performance of this feature. If you experience problems please review your gadgets and user scripts, including global ones." } diff --git a/i18n/qqq.json b/i18n/qqq.json index 9df97a572..304cd2a90 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -38,9 +38,5 @@ "prefs-reading": "Title for 'Reading preferences' section on preferences page; it's a magic message derived from PREVIEWS_PREFERENCES_SECTION (see commit f597e34)", "popups-prefs-optin": "Label for Page Previews option (description for Page Previews option)", "popups-prefs-disable-nav-gadgets-info": "Help message telling to disable the \"Navigation popups\" gadget in order to enable page previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences", - "popups-prefs-conflicting-gadgets-info": "Help message informing about possible conflicts with other gadgets/customizations", - "popups-prefs-navpopups-gadget-conflict-info": "Help message telling to disable the \"Navigation popups\" gadget in order to allow page and reference previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences", - "popups-prefs-reftooltips-gadget-conflict-info": "Help message telling to disable the \"Reference Tooltips\" gadget in order to allow reference previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences", - "popups-prefs-reftooltips-and-navpopups-gadget-conflict-info": "Help message telling to disable the \"Navigation popups\" and \"Reference Tooltips\" gadgets in order to allow page and reference previews. The word \"Gadgets\" should be based on {{msg-mw|prefs-gadgets}}.\n\nParameters:\n* $1 – Link to the Gadgets tab in the user's preferences", - "popups-refpreview-user-preference-label": "Label for the user option to enable or disable reference preview popups" + "popups-prefs-conflicting-gadgets-info": "Help message informing about possible conflicts with other gadgets/customizations" } diff --git a/includes/PopupsContext.php b/includes/PopupsContext.php index ad40e4be8..9d5abd256 100644 --- a/includes/PopupsContext.php +++ b/includes/PopupsContext.php @@ -51,23 +51,10 @@ class PopupsContext { */ public const PREVIEWS_DISABLED = false; - /** - * User preference key to enable/disable Page Previews - */ - public const PREVIEWS_OPTIN_PREFERENCE_NAME = 'popups'; - - /** - * User preference key to enable/disable Reference Previews. Named - * "mwe-popups-referencePreviews-enabled" in localStorage for anonymous users. - */ - public const REFERENCE_PREVIEWS_PREFERENCE_NAME = 'popups-reference-previews'; - /** * Flags passed on to JS representing preferences */ private const NAV_POPUPS_ENABLED = 1; - private const REF_TOOLTIPS_ENABLED = 2; - private const REFERENCE_PREVIEWS_ENABLED = 4; /** * @var Config @@ -145,42 +132,12 @@ class PopupsContext { return $this->config->get( 'PopupsHideOptInOnPreferencesPage' ) === false; } - /** - * @param User $user User whose preferences are checked - * @return bool whether or not to show reference previews - */ - public function isReferencePreviewsEnabled( User $user ) { - return !$user->isNamed() || $this->userOptionsLookup->getBoolOption( - $user, self::REFERENCE_PREVIEWS_PREFERENCE_NAME - ); - } - /** * @param User $user User whose preferences are checked * @return int */ public function getConfigBitmaskFromUser( User $user ) { - return ( $this->conflictsWithNavPopupsGadget( $user ) ? self::NAV_POPUPS_ENABLED : 0 ) | - ( $this->conflictsWithRefTooltipsGadget( $user ) ? self::REF_TOOLTIPS_ENABLED : 0 ) | - ( $this->isReferencePreviewsEnabled( $user ) ? self::REFERENCE_PREVIEWS_ENABLED : 0 ); - } - - /** - * @param User $user User whose preferences are checked - * @return bool - */ - public function shouldSendModuleToUser( User $user ) { - if ( !$user->isNamed() ) { - return true; - } - - $shouldLoadPagePreviews = $this->userOptionsLookup->getBoolOption( - $user, - self::PREVIEWS_OPTIN_PREFERENCE_NAME - ); - $shouldLoadReferencePreviews = $this->isReferencePreviewsEnabled( $user ); - - return $shouldLoadPagePreviews || $shouldLoadReferencePreviews; + return ( $this->conflictsWithNavPopupsGadget( $user ) ? self::NAV_POPUPS_ENABLED : 0 ); } /** diff --git a/includes/PopupsHooks.php b/includes/PopupsHooks.php index 0308d56c5..402506253 100644 --- a/includes/PopupsHooks.php +++ b/includes/PopupsHooks.php @@ -28,7 +28,6 @@ use MediaWiki\Output\Hook\MakeGlobalVariablesScriptHook; use MediaWiki\Output\OutputPage; use MediaWiki\Preferences\Hook\GetPreferencesHook; use MediaWiki\ResourceLoader\Hook\ResourceLoaderGetConfigVarsHook; -use MediaWiki\User\Hook\UserGetDefaultOptionsHook; use MediaWiki\User\Options\UserOptionsManager; use MediaWiki\User\User; use Psr\Log\LoggerInterface; @@ -43,7 +42,6 @@ class PopupsHooks implements GetPreferencesHook, BeforePageDisplayHook, ResourceLoaderGetConfigVarsHook, - UserGetDefaultOptionsHook, MakeGlobalVariablesScriptHook { @@ -101,10 +99,7 @@ class PopupsHooks implements } $skinPosition = array_search( 'skin', array_keys( $prefs ) ); - $readingOptions = array_merge( - $this->getPagePreviewPrefToggle( $user ), - $this->getReferencePreviewPrefToggle( $user ) - ); + $readingOptions = $this->getPagePreviewPrefToggle( $user ); if ( $skinPosition !== false ) { $injectIntoIndex = $skinPosition + 1; @@ -137,43 +132,7 @@ class PopupsHooks implements } return [ - PopupsContext::PREVIEWS_OPTIN_PREFERENCE_NAME => $option - ]; - } - - /** - * Get Reference Preview option - * - * @param User $user User whose preferences are being modified - * @return array[] - */ - private function getReferencePreviewPrefToggle( User $user ) { - $option = [ - 'type' => 'toggle', - 'label-message' => 'popups-refpreview-user-preference-label', - 'help-message' => 'popups-prefs-conflicting-gadgets-info', - 'section' => self::PREVIEWS_PREFERENCES_SECTION - ]; - - $isNavPopupsGadgetEnabled = $this->popupsContext->conflictsWithNavPopupsGadget( $user ); - $isRefTooltipsGadgetEnabled = $this->popupsContext->conflictsWithRefTooltipsGadget( $user ); - - if ( $isNavPopupsGadgetEnabled && $isRefTooltipsGadgetEnabled ) { - $option[ 'disabled' ] = true; - $option[ 'help-message' ] = [ 'popups-prefs-reftooltips-and-navpopups-gadget-conflict-info', - 'Special:Preferences#mw-prefsection-gadgets' ]; - } elseif ( $isNavPopupsGadgetEnabled ) { - $option[ 'disabled' ] = true; - $option[ 'help-message' ] = [ 'popups-prefs-navpopups-gadget-conflict-info', - 'Special:Preferences#mw-prefsection-gadgets' ]; - } elseif ( $isRefTooltipsGadgetEnabled ) { - $option[ 'disabled' ] = true; - $option[ 'help-message' ] = [ 'popups-prefs-reftooltips-gadget-conflict-info', - 'Special:Preferences#mw-prefsection-gadgets' ]; - } - - return [ - PopupsContext::REFERENCE_PREVIEWS_PREFERENCE_NAME => $option + 'popups' => $option ]; } @@ -194,10 +153,7 @@ class PopupsHooks implements return; } - $user = $out->getUser(); - if ( $this->popupsContext->shouldSendModuleToUser( $user ) ) { - $out->addModules( [ 'ext.popups' ] ); - } + $out->addModules( [ 'ext.popups' ] ); } /** @@ -225,8 +181,6 @@ class PopupsHooks implements * the users settings. These variables end in an inline