FoundKeyGang/FoundKey
FoundKeyGang/FoundKey
7
19
Fork 22
Code Issues 37 Pull requests 4 Releases Activity

fix: default-selected reply scopes on client #67

Closed
toast wants to merge 8 commits from feat/default-note-visibility into main
pull from: feat/default-note-visibility
merge into: FoundKeyGang:main
Conversation 2 Commits 8 Files changed 31 +51 -58
toast commented 2022-08-17 21:19:45 +00:00
Owner
Copy link

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
Copy link

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.
packages/client/src/components/post-form.vue Outdated
@ -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 => {
Johann150 commented 2022-08-18 11:55:33 +00:00
Owner
Copy link

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 => { ```
👍 1
toast commented 2022-08-19 11:58:14 +00:00
Author
Owner
Copy link

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.
Johann150 commented 2022-08-19 20:51:06 +00:00
Owner
Copy link

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
packages/client/src/components/post-form.vue Outdated
@ -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"
Johann150 commented 2022-08-18 11:55:46 +00:00
Owner
Copy link

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.
toast commented 2022-08-19 11:57:18 +00:00
Author
Owner
Copy link

Okay, I'll make public the default case.

Okay, I'll make public the default case.
toast marked this conversation as resolved
packages/client/src/components/post-form.vue Outdated
@ -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;
Johann150 commented 2022-08-18 12:05:12 +00:00
Owner
Copy link

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.
toast commented 2022-08-19 11:57:01 +00:00
Author
Owner
Copy link

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).
Johann150 commented 2022-08-19 21:02:44 +00:00
Owner
Copy link

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
toast commented 2022-08-19 21:55:08 +00:00
Author
Owner
Copy link

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?
norm commented 2022-08-31 23:41:24 +00:00
Owner
Copy link

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.
👍 1
toast commented 2022-09-01 12:41:12 +00:00
Author
Owner
Copy link

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
toast commented 2022-08-19 21:55:26 +00:00
Author
Owner
Copy link

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
Details
ci/woodpecker/push/build Pipeline was successful
Details
ci/woodpecker/push/test Pipeline was successful
Details
ci/woodpecker/push/lint-client Pipeline was successful
Details
ci/woodpecker/push/lint-backend Pipeline was successful
Details
ci/woodpecker/pr/lint-foundkey-js Pipeline was successful
Details
ci/woodpecker/pr/lint-backend Pipeline was successful
Details
ci/woodpecker/pr/build Pipeline was successful
Details
ci/woodpecker/pr/lint-client Pipeline failed
Details
ci/woodpecker/pr/test Pipeline failed
Details
8322e7ec2c
toast reviewed 2022-09-14 21:13:48 +00:00
toast left a comment
Author
Owner
Copy link

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
Details
ci/woodpecker/push/build Pipeline was successful
Details
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
Details
ci/woodpecker/push/lint-client Pipeline was successful
Details
ci/woodpecker/push/test Pipeline was successful
Details
e08d804f1b
Johann150 approved these changes 2022-09-15 00:07:46 +00:00
norm commented 2022-09-15 21:45:43 +00:00
Owner
Copy link

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
Details
ci/woodpecker/push/build Pipeline was successful
Details
ci/woodpecker/push/lint-foundkey-js Pipeline was successful
Details
ci/woodpecker/push/lint-client Pipeline was successful
Details
ci/woodpecker/push/test Pipeline was successful
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Johann150
Labels
Clear labels   
feature

task that proposes new feature(s)

  
fix

task to fix behavioral problems in the FoundKey codebase

  
upkeep

task that should make FoundKey smaller and more maintainable

No labels
feature
fix
upkeep
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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.
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?