fix: default-selected reply scopes on client #67
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/default-note-visibility"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
As a side-effect, I remove the broken "remember privacy scope" setting.
There is also a temporary set of functions that should be moved to foundkey-js
later.
Also note that the
visibility
defaultStore parameter doesn't appear to be used anywhere and should be removed.It should have been where the "previous" note visibility gets stored, but that does not appear to actually happen (leading to it always being treated as public).
Setting the visibility to be remembered is done here:
afcf2df3f7/packages/client/src/components/post-form.vue (L399)
Perhaps instead of blanket removing it, it should be fixed.@ -137,1 +137,3 @@
let visibility = $ref(props.initialVisibility ?? (defaultStore.state.rememberNoteVisibility ? defaultStore.state.visibility : defaultStore.state.defaultNoteVisibility) as typeof misskey.noteVisibilities[number]);
// TODO: compat with misskey-js, should likely be moved into foundkey-js
const visibilityScore = (a: string): string => {
The return type is wrong, it should be
number
:I think I'd like to remove it anyways - it isn't particularly intuitive, and I struggle to come up with a good use-case for it.
I'm not sure I understand correctly what you are referencing with "it" here. Or do you really want to remove the return type? (In that case, what would be a use case of a return type?)
@ -138,0 +142,4 @@
case 'home': return 2;
case 'followers': return 1;
case 'specified': return 0;
default: return 4; // invalid visiblity = always considered "too high"
I don't think we have to be quite so paranoid to have a case for invalid visibilities. At best I'd make one of the cases also be the default case.
Okay, I'll make public the default case.
@ -138,0 +145,4 @@
default: return 4; // invalid visiblity = always considered "too high"
}
};
const minVisibility = (a: string, b: string): string => visibilityScore(b) > visibilityScore(a) ? a : b;
If you use an approach more similar to what I did for the backend this function could also be written something like this:
Then the
visibilityScore
function is not needed.I did think of doing that but decided not to for two reasons:
Once foundkey-js is in-repo, ideally we would have such "shared" code there (and have it be a real enum).
true but I thought the idea was to incorporate that sooner rather than later. until then we don't need to update it.
TypeScript has enums but I don't think it would be appropriate to use them here. Especially since we would need to use the note visibilities as an array either way at least for generating the API docs as well.
Well I don't think a switch over strings is that different. Time difference over 2^16 iterations was 5-10 ms depending on hardware, i.e. per iteration roughly 76 to 152 nanoseconds difference. So really, I think this is insignificant for performance.
my benchmark code
yes
no, as I said above
Mmm I still feel more comfortable with this tbh - for purely preference reasons.
It also helps catch arbitrary "wrong" values.
Is it significant to you with the other issues solved?
Now that foundkey-js is part of the repo, maybe it might be worth moving this function (and relevant related code) to that now.
Not sure about the enum thing. I think it's pretty unlikely the ordering of visibility settings would be changed, but it's not really a big deal either way personally.
yeah now that it's in the repo my plan is to rebase and look into it
P.S. Merging here should probably be done as a squash now that there are fixup commits.
027cad7aa0
tof10efdfa8a
f10efdfa8a
toee9173c946
LGTM - let's test and merge.
Rebased and merged in
d1a29ce87e
Pull request closed