PM settings aren't saving properly

Aaron

  • Posts: 356
PM settings aren't saving properly
« on April 5th, 2011, 09:37 AM »
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.)
"The entire British Empire was built on cups of tea … and if you think I'm going to war without one, mate, you're mistaken."

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: PM settings aren't saving properly
« Reply #1, on April 5th, 2011, 09:40 AM »
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 ;)
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: PM settings aren't saving properly
« Reply #2, on April 5th, 2011, 10:29 AM »
(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! -_-

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: PM settings aren't saving properly
« Reply #3, on April 5th, 2011, 10:30 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.

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: PM settings aren't saving properly
« Reply #4, on April 5th, 2011, 10:41 AM »Last edited on April 25th, 2011, 07:45 AM by Nao/Gilles
I believe I found the culprit and fixed it.

In loadThemeOptions (Profile-Modify.php:2125), at the beginning, replace the if block with:

Code: [Select]
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
Quote from Arantor on April 5th, 2011, 10:30 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.
Yes, that's what I was looking into. It failed to update $context['member']['options'], technically. (See above.)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: PM settings aren't saving properly
« Reply #5, on April 5th, 2011, 10:43 AM »
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)

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: PM settings aren't saving properly
« Reply #6, on April 5th, 2011, 10:46 AM »
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

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: PM settings aren't saving properly
« Reply #7, on April 5th, 2011, 10:54 AM »
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

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: PM settings aren't saving properly
« Reply #8, on April 5th, 2011, 10:57 AM »
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...

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

groundup

  • Posts: 25
Re: PM settings aren't saving properly
« Reply #11, on April 27th, 2011, 01:43 AM »
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.

spoogs

  • Posts: 417
Re: PM settings aren't saving properly
« Reply #12, on April 27th, 2011, 04:30 AM »
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.
Stick a fork in it SMF

MultiformeIngegno

  • Posts: 1,337
Re: PM settings aren't saving properly
« Reply #13, on April 27th, 2011, 10:51 AM »
@spoogs: +1. They are confusing and badly done (the "change" "don't change" stuff isn't clear at all..)!

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: PM settings aren't saving properly
« Reply #14, on April 27th, 2011, 11:12 AM »
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.