-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Don't save layout if in dont_override. #9822
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
base: master
Are you sure you want to change the base?
Conversation
PR thomascube/roundcube-elastic4mobile#8 is submitted to make use of that change. |
@@ -62,7 +62,8 @@ public function run($args = []) | |||
// register layout change | |||
if ($layout && preg_match('/^[a-zA-Z_-]+$/', $layout)) { | |||
$rcmail->output->set_env('layout', $layout); | |||
$save_arr['layout'] = $layout; | |||
if (!in_array('layout', $dont_override)) | |||
$save_arr['layout'] = $layout; | |||
// force header replace on layout change |
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.
Please add { }
. Also, what about the other lines of code around? Should set_env()
still be invoked? Should $cols
be set?
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.
The initial issue that I'm trying to fix here is that the elastic4mobile
plugin resets the layout in user preferences every time the user accesses their account from a mobile device. If a user normally uses, say, the Larry skin with the list
layout, as soon as they access the webmail using a mobile browser, the temporarily used Elastic skin resets the layout to widescreen
in a permanent fashion. When the user logs back using a desktop browser, the layout is widescreen
and has to be changed in preferences back to list
.
The thing is that elastic4mobile
is a hack that temporarily overrides the skin to Elastic when the requesting browser is in mobile mode. Further down the road, the currently used skin can change the layout if the current layout is not actually supported by the skin (which is the case with Elastic, which supports only widescreen
). All the rest seems to work just right, (calls to set_env()
, setting of $cols
, and whatnot) except that the layout
setting is saved to the user's preferences.
So it seems reasonable to leave everything else as is and only prevent overwriting of the layout
preference when it is found in dont_override
. On the plugin side, elastic4mobile
must simply add layout
to dont_override
in a config_set
hook, and do this for every request (including AJAX), so that the layout
preference doesn't update the saved user preferences.
This simple change ensures that the layout will not be overwritten to user preferences if
layout
is indont_override
.A plugin can thus manipulate the config by adding a hook to
config_get
in order to make the setting oflayout
temporary. A prime example of a plugin whom this would come in handy iselastic4mobile
, which currently overwrites the layout setting, as reported in #8403. A PR for that plugin is coming shortly.