possible granular settings bug on url previews

[ara4n]

richvdh disabled url previews in the matrix ops room. however, my client continued to show them, claiming i'd explicitly manually enabled them for that room. i'm pretty sure i hadn't.

[turt2live]

@ara4n can you run this bit of js for me? Replacing the targetRoomId with the correct room ID of course:

(function(){
    // urlPreviewsEnabled
    // ['device', 'room-device', 'room-account', 'account', 'config', 'room']

    var targetRoomId = '!JuIwrfKPKiEVFIGqcj:t2l.io';
    var targetRoom = mxMatrixClientPeg.get().getRoom(targetRoomId);

    var previewsAtDevice = JSON.parse((localStorage.getItem("mx_local_settings") || "{}"))["urlPreviewsEnabled"];
    var previewsAtRoomDevice = JSON.parse((localStorage.getItem("mx_setting_urlPreviewsEnabled_" + targetRoomId) || "{}"))['value'];
    var previewsAtRoomAccount = targetRoom ? !(targetRoom.getAccountData("org.matrix.room.preview_urls") || {getContent:function(){return {};}}).getContent()['disable'] : undefined;
    var previewsAtAccount = !(mxMatrixClientPeg.get().getAccountData("org.matrix.preview_urls") || {getContent:function(){return {};}}).getContent()['disable'];
    var previewsAtConfig = ((mxReactSdkConfig || {})["settingDefaults"] || {})["urlPreviewsEnabled"];
    var previewsAtRoom = targetRoom ? !(targetRoom.currentState.getStateEvents("org.matrix.room.preview_urls", "") || {getContent:function(){return{};}}).getContent()['disable'] : undefined;

    console.log("urlPreviewsEnabled @ device = " + previewsAtDevice);
    console.log("urlPreviewsEnabled @ room-device = " + previewsAtRoomDevice);
    console.log("urlPreviewsEnabled @ room-account = " + previewsAtRoomAccount);
    console.log("urlPreviewsEnabled @ account = " + previewsAtAccount);
    console.log("urlPreviewsEnabled @ config = " + previewsAtConfig);
    console.log("urlPreviewsEnabled @ room = " + previewsAtRoom);
})();

All it does is pull the values that Granular Settings should be pulling, just in a more direct way.

Edited: fixed a bug with getting state events

[ara4n]

so i've already had to override my local pref for that room in order to get rid of the url previews, so the data is probably no longer correct:

18:22:17.538 rageshake.js:65 urlPreviewsEnabled @ device = undefined
18:22:17.539 rageshake.js:65 urlPreviewsEnabled @ room-device = undefined
18:22:17.539 rageshake.js:65 urlPreviewsEnabled @ room-account = false
18:22:17.539 rageshake.js:65 urlPreviewsEnabled @ account = true
18:22:17.539 rageshake.js:65 urlPreviewsEnabled @ config = undefined
18:22:17.540 rageshake.js:65 urlPreviewsEnabled @ room = false

[turt2live]

So it looks like what was happening was the account setting of "always show previews" was used because it was an explicit "true". With the levels ordering, this is expected.

Url previews are kinda subjective as to what the ordering should be. In your case, it seems the desired choice is the room setting, however for other rooms people want to show previews anyways, regardless of the room setting.

[ara4n]

hm, so my account default setting of true overrode the room's state config of false? This feels very surprising...

[turt2live]

tbh it'd be surprising under different circumstances if the order was flipped. I'd be interested as to what others think though.

[lampholder]

Bug isnt't really the right label for this - the state is more 'discussion required'.

[lukebarnard1]

I would say that room-account should override the room. @turt2live what is the order of precedence here?..

so i've already had to override my local pref for that room in order to get rid of the url previews, so the data is probably no longer correct:

oh, Matthew says that this is being honoured at least

[lukebarnard1]

Url previews are kinda subjective as to what the ordering should be

I really hope the ordering is the same for any setting! If that's not true, the model of granularity might need a rethink to combat complexity like this.

[turt2live]

The order is the same as the JS output:

['device', 'room-device', 'room-account', 'account', 'config', 'room']

This is the same order for everything that has LEVELS_ROOM_SETTINGS_WITH_ROOM ( https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/settings/Settings.js#L27 ).

The room has always been designed to come after the account so that the user has more control over their client. For instance, if a room dictates that it should be tinted red, but the user wants all their rooms to be blue then the rooms should be blue. Similarly, for URL previews a user may want to globally disable them (or globally enable them) for their account. The intent is to always give the user's choice precedence over someone else's.

The config level was the only one that had actually shifted position between the initial design and merge (all the other changes were done prior to the initial design). This was because the build config ended up being more useful to target a group of users (everyone on the instance), and therefore has a narrow scope compared to a room (which targets everyone) in a way. It doesn't mean much for instances like riot.im/app because the config affects many more people than a room would, however people running a private instance would generally want to pick sensible defaults for their users (such as communities where they deem avatar changes too noisy and disable them by default for everyone). For reference, it was originally [account, room, config]. This was changed to be [account, config, room] for the reasons described.

Back to why room comes after account though: The whole idea is to prevent room admins from making your client behave in a way that is completely unexpected or not desired. The same applies for why room admins can only change 6 of the settings in their room (url previews, tint, hiding redactions, hiding join/leave events, hiding avatar changes, and hiding display name changes).

More review is probably needed, but I thought I should actually just dump the reasoning here instead of hinting at it over and over.

[lukebarnard1]

@turt2live, I have a separate question: If I haven't ever changed the setting at any level, presumably it takes the default value. But there seemingly isn't a way to revert a setting to "default" as in "use the previous setting by precedence"? I feel like part of the confusion could be coming from this; the checkbox UI isn't really suited to this kind of data (it needs a pass-thru setting).

[turt2live]

Correct, riot doesn't provide unsetting it. The intent of the original system was to have very little UX impact because tri-state checkboxes could be confusing. I think @lampholder had thoughts on how the UX of granular settings could work

[t3chguy]

This may have been originally caused by a typo in GranularSettings which was fixed by matrix-org/matrix-react-sdk@d1600bd

[t3chguy]

I believe this was fixed above, re-open if ever reproduced