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

Share user settings for persistent choices made in the UI #13466

Closed
nijel opened this Issue Jul 10, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@nijel
Member

nijel commented Jul 10, 2017

Currently there are several approaches used in the code to ensure persistence of some settings. I think there should be really just one way to do that and that should not use much of the cookies (see #11688).

Current (and planned settings):

  • Console settings, stored as JSON in cookie:

    phpmyadmin/js/console.js

    Lines 408 to 423 in 80e90bf

    updateConfig: function () {
    PMA_console.config = {
    alwaysExpand: $('#pma_console_options input[name=always_expand]').prop('checked'),
    startHistory: $('#pma_console_options').find('input[name=start_history]').prop('checked'),
    currentQuery: $('#pma_console_options').find('input[name=current_query]').prop('checked'),
    enterExecutes: $('#pma_console_options').find('input[name=enter_executes]').prop('checked'),
    darkTheme: $('#pma_console_options').find('input[name=dark_theme]').prop('checked')
    };
    Cookies.set('pma_console_config', PMA_console.config);
    /* Setting the dark theme of the console*/
    if (PMA_console.config.darkTheme) {
    $('#pma_console').find('>.content').addClass('console_dark_theme');
    } else {
    $('#pma_console').find('>.content').removeClass('console_dark_theme');
    }
    },
  • Console mode, stored as single cookie:

    phpmyadmin/js/console.js

    Lines 69 to 71 in 80e90bf

    if (! Cookies.get('pma_console_mode')) {
    Cookies.set('pma_console_mode', 'info');
    }
  • Console debug, stored as single cookie:
    Cookies.set('pma_console_dbg_config', this._config);
  • Console height, stored as single cookie:

    phpmyadmin/js/console.js

    Lines 66 to 68 in 80e90bf

    if (! Cookies.get('pma_console_height')) {
    Cookies.set('pma_console_height', 92);
    }
  • Navigation width, stored as single cookie:
    Cookies.set('pma_navi_width', event.data.resize_handler.getPos(event));
  • Navigation bar status, see #13461 and #13397
  • Mobile/Desktop mode, see #13422 (comment)
  • Connection collation, stored as single cookie:
    if (isset($GLOBALS['collation_connection'])) {
    $GLOBALS['PMA_Config']->setCookie(
    'pma_collation_connection',
    $GLOBALS['collation_connection']
    );
    }

    if (isset($config_data[$collation])) {
    $GLOBALS[$collation]
    = $config_data[$collation];
    $this->setCookie(
    'pma_collation_connection',
    $GLOBALS[$collation]
    );
  • User interface language, stored as single cookie:
    $this->setCookie('pma_lang', $language->getCode());
  • User interface font size, stored as single cookie:
    $this->setCookie('pma_fontsize', $this->get('fontsize'), '82%');
  • Export template defaults, stored as three cookies:
    if ($export_type == 'server') {
    if (! empty($remember_template)) {
    $GLOBALS['PMA_Config']->setUserValue(
    'pma_server_filename_template',
    'Export/file_template_server',
    $filename_template
    );
    }
    } elseif ($export_type == 'database') {
    if (! empty($remember_template)) {
    $GLOBALS['PMA_Config']->setUserValue(
    'pma_db_filename_template',
    'Export/file_template_database',
    $filename_template
    );
    }
    } else {
    if (! empty($remember_template)) {
    $GLOBALS['PMA_Config']->setUserValue(
    'pma_table_filename_template',
    'Export/file_template_table',
    $filename_template
    );
    }
    }
  • phpMyAdmin cookie version, stored as single cookie, which is supposed to handle upgrades of all this mess:
    /**
    * clean cookies on upgrade
    * when changing something related to PMA cookies, increment the cookie version
    */
    $pma_cookie_version = 5;
    if (isset($_COOKIE)) {
    if (! isset($_COOKIE['pmaCookieVer'])
    || $_COOKIE['pmaCookieVer'] != $pma_cookie_version
    ) {
    // delete all cookies
    foreach ($_COOKIE as $cookie_name => $tmp) {
    // We ignore cookies not with pma prefix
    if (strncmp('pma', $cookie_name, 3) != 0) {
    continue;
    }
    $GLOBALS['PMA_Config']->removeCookie($cookie_name);
    }
    $_COOKIE = array();
    $GLOBALS['PMA_Config']->setCookie('pmaCookieVer', $pma_cookie_version);
    }
    }

My proposal:

  • Make all these standard configuration options
  • Expose them using user configuration
  • Remove single cookie usage from Config::setUserValue
  • Provide javascript API to change user settings (which will call Config::setUserValue in the end)

This way the settings will persist in a best way available (see https://docs.phpmyadmin.net/en/latest/settings.html) and user will be able to manipulate with their default values as with other settings.

Further improvements:

  • Automatically load user settings from browser local storage (currently this requires user action), see #13843
  • Alternatively we might add fallback of user settings to save changed values to a single cookie, but I'm not sure using cookies is a good idea for this as they are sent with every request and that can make it easy to hit headers limit (see phpmyadmin/docker#32).

Note: Overall this leads to 12 settings cookies + 2 authentication cookies per server.

@ibennetch

This comment has been minimized.

Show comment
Hide comment
@ibennetch

ibennetch Jul 11, 2017

Member

This is a great idea.

Some of these are things that should apply per-device, such as whether a user sees the mobile or desktop version of the interface. It's not ideal to store such things as a configuration directive or through the user preferences table, because they should not apply to all of the devices a user may use. On the other hand, things like the export template defaults are specific to a user, so they should persist for a specific user no matter which device they use.

Member

ibennetch commented Jul 11, 2017

This is a great idea.

Some of these are things that should apply per-device, such as whether a user sees the mobile or desktop version of the interface. It's not ideal to store such things as a configuration directive or through the user preferences table, because they should not apply to all of the devices a user may use. On the other hand, things like the export template defaults are specific to a user, so they should persist for a specific user no matter which device they use.

@udan11

This comment has been minimized.

Show comment
Hide comment
@udan11

udan11 Jul 11, 2017

Member

I believe we should have something like this:

/**
 * Sets a configuration value.
 *
 * A configuration value may be set in both browser's local storage and
 * remotely in server's configuration table.
 *
 * If the `only_local` argument is `true`, the value is store is stored only in
 * browser's local storage and may be lost if the user resets his browser's
 * settings.
 *
 * NOTE: Depending on server's configuration, the configuration table may be or
 * not persistent.
 *
 * @param  {string}     key         Configuration key.
 * @param  {object}     value       Configuration value.
 * @param  {boolean}    only_local  Configuration type.
 */
function config_set_value(key, value, only_local=false)
{
    let serialized = JSON.stringify(value);
    localStorage.setItem(key, serialized);
    $.ajax({
        url: "configuration.php",
        type: "POST",
        data: {
            key: key,
            value: serialized,
        }
    });
}

/**
 * Gets a configuration value. A configuration value will be searched in
 * browser's local storage first and if not found, a call to the server will be
 * made.
 *
 * If value should not be cached and the up-to-date configuration value from
 * right from the server is required, the third parameter should be `false`.
 *
 * @param  {string}     key         Configuration key.
 * @param  {boolean}    only_local  Configuration type.
 *
 * @return {object}                 Configuration value.
 */
function config_get_value(key, cached=true)
{
    let value = localStorage.getItem(key);
    if (cached && value !== undefined) {
        return JSON.parse(value);
    }

    // Result not found in local storage or ignored.
    // Hitting the server.
    $.ajax({
        // TODO: This is ugly, but usually when a configuration is needed,
        // processing cannot continue until that value is found.
        // Another solution is to provide a callback as a parameter.
        async: false,
        url: "configuration.php",
        data: {
            key: key
        },
        success: function (data) {
            // Updating value in local storage.
            localStorage.setItem(key, data);
            // Eventually, call callback.
        }
    });
    return JSON.parse(localStorage.getItem(key));
}

This way, we can also store per-device configuration as long as they are only in browser's local storage (only_local=true).

Is this a good starting point? Did I get things right?

By the way, there is some code that handles configuration in js/config.js but it's quite tightly coupled with configuration forms and on user preferences pages (also looks hacky).

Member

udan11 commented Jul 11, 2017

I believe we should have something like this:

/**
 * Sets a configuration value.
 *
 * A configuration value may be set in both browser's local storage and
 * remotely in server's configuration table.
 *
 * If the `only_local` argument is `true`, the value is store is stored only in
 * browser's local storage and may be lost if the user resets his browser's
 * settings.
 *
 * NOTE: Depending on server's configuration, the configuration table may be or
 * not persistent.
 *
 * @param  {string}     key         Configuration key.
 * @param  {object}     value       Configuration value.
 * @param  {boolean}    only_local  Configuration type.
 */
function config_set_value(key, value, only_local=false)
{
    let serialized = JSON.stringify(value);
    localStorage.setItem(key, serialized);
    $.ajax({
        url: "configuration.php",
        type: "POST",
        data: {
            key: key,
            value: serialized,
        }
    });
}

/**
 * Gets a configuration value. A configuration value will be searched in
 * browser's local storage first and if not found, a call to the server will be
 * made.
 *
 * If value should not be cached and the up-to-date configuration value from
 * right from the server is required, the third parameter should be `false`.
 *
 * @param  {string}     key         Configuration key.
 * @param  {boolean}    only_local  Configuration type.
 *
 * @return {object}                 Configuration value.
 */
function config_get_value(key, cached=true)
{
    let value = localStorage.getItem(key);
    if (cached && value !== undefined) {
        return JSON.parse(value);
    }

    // Result not found in local storage or ignored.
    // Hitting the server.
    $.ajax({
        // TODO: This is ugly, but usually when a configuration is needed,
        // processing cannot continue until that value is found.
        // Another solution is to provide a callback as a parameter.
        async: false,
        url: "configuration.php",
        data: {
            key: key
        },
        success: function (data) {
            // Updating value in local storage.
            localStorage.setItem(key, data);
            // Eventually, call callback.
        }
    });
    return JSON.parse(localStorage.getItem(key));
}

This way, we can also store per-device configuration as long as they are only in browser's local storage (only_local=true).

Is this a good starting point? Did I get things right?

By the way, there is some code that handles configuration in js/config.js but it's quite tightly coupled with configuration forms and on user preferences pages (also looks hacky).

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Jul 12, 2017

Member

@udan11 This is pretty much what I was thinking of. Using local storage is IMHO better than cookies for this purpose as this doesn't inflate requests size. The only question is whether we would need some of the local (per-device) settings on the server side, but I think there shouldn't be any such setting right now.

PS: I'd rather reuse existing ajax.php endpoint instead of adding another one for this (configuration.php).

Member

nijel commented Jul 12, 2017

@udan11 This is pretty much what I was thinking of. Using local storage is IMHO better than cookies for this purpose as this doesn't inflate requests size. The only question is whether we would need some of the local (per-device) settings on the server side, but I think there shouldn't be any such setting right now.

PS: I'd rather reuse existing ajax.php endpoint instead of adding another one for this (configuration.php).

@nijel nijel self-assigned this Aug 2, 2017

@nijel nijel added this to the 4.8.0 milestone Aug 2, 2017

nijel added a commit to nijel/phpmyadmin that referenced this issue Aug 2, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Aug 2, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

@nijel nijel referenced this issue Aug 2, 2017

Merged

User configuration handling (base code) #13551

4 of 4 tasks complete
@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Aug 2, 2017

Member

@udan11 I've started with your code and I'm working on it in #13551

Member

nijel commented Aug 2, 2017

@udan11 I've started with your code and I'm working on it in #13551

nijel added a commit to nijel/phpmyadmin that referenced this issue Aug 2, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Aug 2, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Sep 4, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Oct 10, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Oct 10, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Oct 10, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 28, 2017

Initial javascript API for handling user configuration
There ar now configGet/configSet functions which can be used to
manipulate user settings.

Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Nov 28, 2017

Member

Another related issue is #13480

Member

nijel commented Nov 28, 2017

Another related issue is #13480

nijel added a commit that referenced this issue Nov 28, 2017

Add console preferences
This will allow to use console preferences using normal userconfig
framework.

Issue #13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store console mode in userconfig
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store console height in user preferences
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store console configuration in user preferences
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Generalize code for console preferences
We can reuse and better cache locally the operations to avoid additional
roundtrip to server when changing settings.

Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Integrate debug console settings into user preferences
Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Remove special casing for font size
It is now stored in configuration in same way as any other setting.

Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Store navigation width in user preferences
There is no need to have this separetely.

Issue #13466
Issue #11688

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit that referenced this issue Nov 28, 2017

Remember state of navigation panel
The NavigationPanel width is now 0 when panel should be collapsed.

Issue #13466
Fixes #13397

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 29, 2017

Move collation connection setting to user preferences
It is now handled same way as other user settings.

Issue phpmyadmin#11688
Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 29, 2017

Move collation connection setting to user preferences
It is now handled same way as other user settings.

Issue phpmyadmin#11688
Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>

nijel added a commit to nijel/phpmyadmin that referenced this issue Nov 29, 2017

Move collation connection setting to user preferences
It is now handled same way as other user settings.

Issue phpmyadmin#11688
Issue phpmyadmin#13466

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Nov 29, 2017

Member

I think we're now done with this.

The only cookies based settings left is language. As we want this to be used on the login screen as well, I see no other way than storing it on the client.

Member

nijel commented Nov 29, 2017

I think we're now done with this.

The only cookies based settings left is language. As we want this to be used on the login screen as well, I see no other way than storing it on the client.

@nijel nijel closed this Nov 29, 2017

nijel added a commit that referenced this issue Nov 29, 2017

Changelog entries for #11688 and #13466
Signed-off-by: Michal Čihař <michal@cihar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment