fix: default-selected reply scopes on client #67

Closed
toast wants to merge 8 commits from feat/default-note-visibility into main
Owner

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).

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).
Johann150 requested changes 2022-08-18 12:15:34 +00:00
Johann150 left a comment
Owner

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.

Setting the visibility to be remembered is done here: https://akkoma.dev/FoundKeyGang/FoundKey/src/commit/afcf2df3f7a72d8a2ac1ec538583edb19a5c2dfa/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 => {
Owner

The return type is wrong, it should be number:

-const visibilityScore = (a: string): string => {
+const visibilityScore = (a: string): number => {
The return type is wrong, it should be `number`: ```diff -const visibilityScore = (a: string): string => { +const visibilityScore = (a: string): number => { ```
Author
Owner

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 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.
Owner

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?)

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?)
toast marked this conversation as resolved
@ -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"
Owner

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.

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.
Author
Owner

Okay, I'll make public the default case.

Okay, I'll make public the default case.
toast marked this conversation as resolved
@ -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;
Owner

If you use an approach more similar to what I did for the backend this function could also be written something like this:

misskey.noteVisibilities[Math.max(
    misskey.noteVisibilities.indexOf(a),
    misskey.noteVisibilities.indexOf(b)
)]

Then the visibilityScore function is not needed.

If you use an approach more similar to what I did for the backend this function could also be written something like this: ```typescript misskey.noteVisibilities[Math.max( misskey.noteVisibilities.indexOf(a), misskey.noteVisibilities.indexOf(b) )] ``` Then the `visibilityScore` function is not needed.
Author
Owner

I did think of doing that but decided not to for two reasons:

  1. This way depends on internal implementation details in misskey-js - if the elements were reordered, or the type changed (e.g to an enum, not sure if TS has those) or similar, it would break.
  2. Posting notes takes long enough as it is, so I'd rather not iterate over (even a small) arrays twice over just for that - a direct lookup is fine.

Once foundkey-js is in-repo, ideally we would have such "shared" code there (and have it be a real enum).

I did think of doing that but decided not to for two reasons: 1. This way depends on internal implementation details in misskey-js - if the elements were reordered, or the type changed (e.g to an enum, not sure if TS has those) or similar, it would break. 2. Posting notes takes long enough as it is, so I'd rather not iterate over (even a small) arrays twice over just for that - a direct lookup is fine. Once foundkey-js is in-repo, ideally we would have such "shared" code there (and have it be a real enum).
Owner

depends on internal implementation details in misskey-js

true but I thought the idea was to incorporate that sooner rather than later. until then we don't need to update it.

type changed (e.g to an enum, not sure if TS has those)

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.

Posting notes takes long enough as it is, so I'd rather not iterate over (even a small) arrays twice over just for that - a direct lookup is fine.

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
const noteVisibilities = ['public', 'home', 'followers', 'specified'];

function johann(a, b) {
	return noteVisibilities[Math.max(
		noteVisibilities.indexOf(a),
		noteVisibilities.indexOf(b)
	)];
}

const visibilityScore = (a) => {
	switch (a) {
		case 'specified': return 0;
		case 'followers': return 1;
		case 'home': return 2;
		case 'public': default: return 3;
	}
};

const minVisibility = (a, b) => visibilityScore(b) > visibilityScore(a) ? a : b;

function randElem(array) {
	return array[Math.floor(Math.random() * array.length)];
}

const tests = [];
for (let i = 0; i < 2**16; i++) {
	tests[i] = [randElem(noteVisibilities), randElem(noteVisibilities)];
}
console.log(tests);

performance.mark('start');

tests.forEach(([a, b]) => johann(a, b));

performance.mark('end');
console.log(performance.measure('johann', 'start', 'end'));

performance.mark('start');

tests.forEach(([a, b]) => minVisibility(a, b));

performance.mark('end');
console.log(performance.measure('tosti', 'start', 'end'));

Once foundkey-js is in-repo, ideally we would have such "shared" code there

yes

(and have it be a real enum).

no, as I said above

> depends on internal implementation details in misskey-js true but I thought the idea was to incorporate that sooner rather than later. until then we don't need to update it. > type changed (e.g to an enum, not sure if TS has those) 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. > Posting notes takes long enough as it is, so I'd rather not iterate over (even a small) arrays twice over just for that - a direct lookup is fine. 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. <details><summary>my benchmark code</summary> ```javascript const noteVisibilities = ['public', 'home', 'followers', 'specified']; function johann(a, b) { return noteVisibilities[Math.max( noteVisibilities.indexOf(a), noteVisibilities.indexOf(b) )]; } const visibilityScore = (a) => { switch (a) { case 'specified': return 0; case 'followers': return 1; case 'home': return 2; case 'public': default: return 3; } }; const minVisibility = (a, b) => visibilityScore(b) > visibilityScore(a) ? a : b; function randElem(array) { return array[Math.floor(Math.random() * array.length)]; } const tests = []; for (let i = 0; i < 2**16; i++) { tests[i] = [randElem(noteVisibilities), randElem(noteVisibilities)]; } console.log(tests); performance.mark('start'); tests.forEach(([a, b]) => johann(a, b)); performance.mark('end'); console.log(performance.measure('johann', 'start', 'end')); performance.mark('start'); tests.forEach(([a, b]) => minVisibility(a, b)); performance.mark('end'); console.log(performance.measure('tosti', 'start', 'end')); ``` </details> > Once foundkey-js is in-repo, ideally we would have such "shared" code there yes > (and have it be a real enum). no, as I said above
Author
Owner

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?

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?
Owner

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.

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.
Author
Owner

yeah now that it's in the repo my plan is to rebase and look into it

yeah now that it's in the repo my plan is to rebase and look into it
Johann150 marked this conversation as resolved
Author
Owner

P.S. Merging here should probably be done as a squash now that there are fixup commits.

P.S. Merging here should probably be done as a squash now that there are fixup commits.
Johann150 force-pushed feat/default-note-visibility from 027cad7aa0 to f10efdfa8a 2022-09-14 09:51:49 +00:00 Compare
Johann150 force-pushed feat/default-note-visibility from f10efdfa8a to ee9173c946 2022-09-14 09:58:02 +00:00 Compare
Johann150 added 1 commit 2022-09-14 20:57:06 +00:00
include renote in visibility computation
Some checks failed
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/test Pipeline was successful
ci/woodpecker/push/lint-client Pipeline was successful
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
8322e7ec2c
toast reviewed 2022-09-14 21:13:48 +00:00
toast left a comment
Author
Owner

LGTM - let's test and merge.

LGTM - let's test and merge.
Johann150 added 1 commit 2022-09-15 00:00:58 +00:00
fix typo
All checks were successful
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
ci/woodpecker/push/lint-client Pipeline was successful
ci/woodpecker/push/test Pipeline was successful
e08d804f1b
Johann150 approved these changes 2022-09-15 00:07:46 +00:00
Owner

Rebased and merged in d1a29ce87e

Rebased and merged in https://akkoma.dev/FoundKeyGang/FoundKey/commit/d1a29ce87e50147fe2d05470abb994393dc7d07f
norm closed this pull request 2022-09-15 21:45:43 +00:00
norm deleted branch feat/default-note-visibility 2022-09-15 21:45:51 +00:00
All checks were successful
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
ci/woodpecker/push/lint-client Pipeline was successful
ci/woodpecker/push/test Pipeline was successful

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
feature
fix
upkeep
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: FoundKeyGang/FoundKey#67
No description provided.