Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: regular expressions in word mutes #8254

Merged
merged 7 commits into from
Feb 10, 2022

Conversation

Johann150
Copy link
Contributor

@Johann150 Johann150 commented Feb 4, 2022

What

  • Handle SyntaxExceptions from faulty RegExps in word mutes.
  • Prevent such exceptions from occurring in the first place by
    • checking that regular expressions in word mutes have valid syntax in the frontend and
    • checking that regular expressions in hard word mutes have valid syntax in the backend (endpoint api/i/update).
  • Clean up empty lines etc. when setting word mutes instead of when checking each note. This should improve perfomance.
  • Correctly handle regular expressions containing spaces: Before they would be split at spaces like other lines, causing them not to be recognized.

Why

Handling exceptions fixes #8245.
Preventing such exceptions improves the user experience.

Additional info

The storage format of muted words will be changed to Array<string | string[]>, where an array element of type string represents a regular expression, and string[] represents a normal word mute.

Hard word mutes can be migrated, but soft word mutes may stop working correctly if they contain RegExps or empty lines. This can be fixed by going to soft word mute settings and saving once. This will update the structure to the new format.

To do

  • Fix behaviour of frontend.
  • Fix behaviour of backend
    • migration for hard word mutes.
  • New i18n strings need to be added. English version below, Japanese version welcome. 🙏
regexpError: "Regular Expression error"
regexpErrorDescription: "Error in the regular expression on line {line} in your {tab} word mutes:"

@robflop
Copy link
Contributor

robflop commented Feb 4, 2022

Suggestions for japanese strings:

regexpError: "正規表現エラー"
regexErrorDescription: "{tab}ワードミュートの{line}列の正規表現にエラーが発生しました:"

Not completely sure what the {tab} bit refers to, so this might be wrong.

@Johann150
Copy link
Contributor Author

Not completely sure what the {tab} bit refers to, so this might be wrong.

It will be either _wordMute.soft or _wordMute.hard, so they know which tab the line number is from.

@robflop
Copy link
Contributor

robflop commented Feb 4, 2022

Ah, thanks. In this case, the string should be suitable.

locales/ja-JP.yml Outdated Show resolved Hide resolved
packages/client/src/pages/settings/word-mute.vue Outdated Show resolved Hide resolved
packages/client/src/scripts/check-word-mute.ts Outdated Show resolved Hide resolved
@Johann150 Johann150 force-pushed the regexp-error branch 2 times, most recently from 3a20ff1 to fcb245b Compare February 4, 2022 22:25
@Johann150 Johann150 marked this pull request as ready for review February 4, 2022 22:32
@syuilo syuilo self-requested a review February 6, 2022 07:05
Copy link
Member

@syuilo syuilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mei23
Copy link
Contributor

mei23 commented Feb 6, 2022

Approvedなら別にいいけど、マイグレーションって必須?
この辺の対応項目ってIssueで最初に議論するんじゃなかったのだわ?

@syuilo
Copy link
Member

syuilo commented Feb 6, 2022

理想的には実装前にIssueで実装方針の認識が合わされてた方がベターだね

@mei23

This comment was marked as off-topic.

@syuilo syuilo merged commit afb6304 into misskey-dev:develop Feb 10, 2022
@syuilo
Copy link
Member

syuilo commented Feb 10, 2022

🙏

@tamaina
Copy link
Contributor

tamaina commented Feb 11, 2022

ソフトミュートを設定しているとバグる(が私はまだコードを呑み込めていない)

2022-02-11 21-20-07 1

@tamaina
Copy link
Contributor

tamaina commented Feb 11, 2022

check-word-muteのmatched内のreturn falseが問題やな

@syuilo
Copy link
Member

syuilo commented Feb 11, 2022

ミュート設定しなおすと直ると思う

@tamaina
Copy link
Contributor

tamaina commented Feb 11, 2022

ミュート設定しなおすと直ると思う

それはいただけない
ブラウザコンソールとか開かない限り設定前の値が出てこないし

@tamaina
Copy link
Contributor

tamaina commented Feb 11, 2022

とりあえずreturn falseせずに後方互換性を確保したい

@tamaina
Copy link
Contributor

tamaina commented Feb 11, 2022

いやなんか勘違いしてた

@tamaina
Copy link
Contributor

tamaina commented Feb 11, 2022

(return falseのせいではなかった

@Johann150 Johann150 deleted the regexp-error branch March 12, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timeline loads old posts but not new posts, notifications work
7 participants