-
Not all PM settings are saved properly when switching to conversation mode. (This is a bug with SMF, too.)
Steps to reproduce:
- create new account on vanilla install
- profile -> PM settings
- Display personal messages as 'a conversation', get an alert
- tick 'Show a popup when I receive new messages.' and 'Save a copy of each personal message in my sent items by default.'
- save settings
- note the display mode being saved, but the checks aren't
On a side note, I think these should be default settings. (I don't think the other display modes should be supported still either, but that's another story.)
-
Well... save a copy should be defaulted to on, and of display mode, one at a time should be the default now (I'm still not entirely convinced about convo mode, but either one at a time or convo mode is an improvement over all-at-once)
I'd also note that the switch UI was pulled to the main view as a dropdown rather than a toggle button for usability purposes ;)
-
(I'm not using the tracker these days, because it kinda takes off my happiness considering I already have a long personal to-do list :P)
Okay, I tried this with my admin account and the "Remove inbox label" option, and it seems that it doesn't save settings *the first time*. As soon as I reproduced the problem, it never occurred again.
Confirmed. I just removed the settings from the DB and tried again, it didn't save but only the first time.
Profile-Modify.php is more complicated than it should be, what with the whole validation thing in an array... Why not simply do the validation in the relevant function? I don't know... Probably because it looks geeky that way?
Okay, am a bit lost... Just analyzing the query data should give me the answer, but... No?!
In both cases (first save, subsequent saves), the generated queries are:
REPLACE INTO wedge_themes(`id_member`, `id_theme`, `variable`, `value`)
VALUES
(1, 1, SUBSTRING('view_newest_pm_first', 1, 255), SUBSTRING('1', 1, 65534)),
(1, 1, SUBSTRING('popup_messages', 1, 255), SUBSTRING('1', 1, 65534)),
(1, 1, SUBSTRING('copy_to_outbox', 1, 255), SUBSTRING('1', 1, 65534)),
(1, 1, SUBSTRING('pm_remove_inbox_label', 1, 255), SUBSTRING('1', 1, 65534))
DELETE FROM wedge_themes
WHERE id_theme != 1
AND variable IN ('view_newest_pm_first', 'popup_messages', 'copy_to_outbox', 'pm_remove_inbox_label')
AND id_member = 1
There's no reason why the second query would delete the first round of set options. Not sure exactly... :-/
Posted: April 5th, 2011, 10:13 AM
Oh... The waste of time.
It actually saves the data correctly. It just doesn't show it on the first reload! -_-
-
That would imply that it's grabbed the info it's going to use (in $options ?), done what it's going to do but failed to update $options before going to the template.
-
I believe I found the culprit and fixed it.
In loadThemeOptions (Profile-Modify.php:2125), at the beginning, replace the if block with:
if ($context['user']['is_owner'])
{
$context['member']['options'] = $options;
if (isset($_POST['options']))
foreach ($_POST['options'] as $k => $v)
$context['member']['options'][$k] = $v;
}
That makes sense. The original code went through all of the options, and would replace them with the new values only if it was set in the first place. I don't believe there's any reason to do that -- it should insert the options without question.
Posted: April 5th, 2011, 10:41 AM
That would imply that it's grabbed the info it's going to use (in $options ?), done what it's going to do but failed to update $options before going to the template.
-
Yeah, that makes sense. Wasn't sure if it was $options or not, but it makes sense that it isn't where theme options are concerned.
(Though, quite honestly, I'm not sure it's the sort of thing that should be a *theme* option, but that's another story)
-
Technically, I can't think of many things that should be a theme option... I'd even tend to say we could remove the id_theme field from that table and rename the table to something else, while we're at it... Theme settings could even be stored in a serialized field in the members table if we really wanted to go through this :P
-
It's been bugging me for a while, actually.
There are some arguments for per-theme options when you combine it with per-board themes, for example it might be that you have one board where you want latest-post-first. But PM settings shouldn't be theme settings, neither should most of look & layout. I am struggling to figure out what should be :/
There is one benefit, though, it makes it relatively easy to scoop up theme stuff, user stuff and custom fields all at once, but even then it's done as multiple queries IIRC
-
I'll let you sleep on it... Maybe we'll come up with a better way to do these things at some point. :)
But if we're sure to change it before the final release, we should rename the table now, to something more generic, before we forget.
Posted: April 5th, 2011, 10:57 AM
Back to AeMe now...
-
Should I move this to the public area or not? (If anything, out of mercy for SiNaN who seems to be struggling with it(http://dev.simplemachines.org/mantis/view.php?id=4675) :P)
-
Go nuts :)
-
I haven't looked at it, but why wouldn't that work? If new_settings == old_settings ? continue : replace;
On a side note, completely unrelated but it doesn't deserve a topic: I love "wedgees" as a group name.
-
I honestly don't understand why most if not all of those are theme options at all... I cant think of a single situation where I wouldn't expect the same options across all themes. I mean I cant see a user wanting their options to be different based on the theme, I just see all of those as user options/preferences and that's it.
I hardly even understand why some (maybe all) of the theme settings are theme related either. SMF has these options out of the box so once set I would expect that to be it, if I install a new theme I'd expect it to adapt the settings. So far only Bloc's themes IMO have a reason to add theme settings since his premium themes actually adds settings not provided by default.
-
@spoogs: +1. They are confusing and badly done (the "change" "don't change" stuff isn't clear at all..)!
-
The bulk of the reason is architectural, rather than anything else; there's nowhere else in SMF to store arbitrary-size, per-user information other than in the theme options area.
I can also see a few of the options actually being worthwhile to have the different options but the vast bulk, no.
And the change/don't change... ick.
-
Which ones do you see worth being theme options.
I actually dont mind where we have to go to set the options, I just don't think they need to be per theme... leave them there but just have one list of options instead of being theme specific... tho I'd rather a tab under Features and Options for 'Default Preferences' or 'Default Member Options'
-
Some things make sense to be non theme specific, like whether to use quick reply.
Some things, though, should be theme specific, like the news fader settings because different themes handle them in different ways. Thing is, do we separate the storage?
-
I agree with you on the news fader Arantor; however it's something I use to use until I installed a theme that it just looked like crap on, but that only lead me to disable it in all themes instead. That even lead me to overall stop using the news all together a use a board for that instead.
Personally I only think the theme settings make sense if a theme is adding new settings not for the settings that are provided by default.