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

無題シーケンス番号の保存に失敗する不具合の暫定対策 #1850

Merged
merged 2 commits into from Jun 9, 2022

Conversation

sanomari
Copy link
Contributor

@sanomari sanomari commented Jun 1, 2022

PR の目的

#1845 で報告された不具合に対し暫定対策を行います。

カテゴリ

  • 不具合修正

PR の背景

現在のmasterに、ユーザビリティに影響する不具合が見つかっています。

PR のメリット

PR のデメリット (トレードオフとかあれば)

仕様・動作説明

#1845 (comment)

拡張領域の指定に「sizeof式」を使っていたために #1794 の修正時に利用箇所を見落としたと考えられます。

領域不足となっている拡張領域に必要な量を指定すると同時に、
「sizeof式」に対応する独自定数を切って今後の見落としを防ぎます。

PR の影響範囲

わかりません。

テスト内容

テスト1

手順
タブモードでCtrl+Nを連打してタブをたくさん開きます。
タブを切り替えても無題番号が消えなくなることを確認しました。

関連 issue, PR

参考資料

拡張領域の指定に「sizeof式」を使っていたために利用箇所を見落とした?
独自定数を切って見落としを防ぎ、必要な量の拡張領域を確保するよう修正。
@sanomari sanomari added the 🐛bug🦋 ■バグ修正(Something isn't working) label Jun 1, 2022
@AppVeyorBot
Copy link

Build sakura 1.0.4141 completed (commit 94026ecb2d by @sanomari)

@k-takata
Copy link
Member

k-takata commented Jun 2, 2022

拡張領域を2個確保する必要はあるのでしょうか?
1個目は #1794GWLP_USERDATA に移ったのかなと思いましたが。

@sanomari
Copy link
Contributor Author

sanomari commented Jun 2, 2022

拡張領域を2個確保する必要はあるのでしょうか? 1個目は #1794GWLP_USERDATA に移ったのかなと思いましたが。

究極的には、拡張領域を確保する必要はありません。
そこに至る道順を解説するのがしんどいので、とりあえずの暫定対策を打つ次第です。

こんな修正を入れるくらいならバグってたほうがマシという考え方もあると思います。

#1794でインデックス0の拡張領域を利用しなくなったので、無題シーケンスを格納する拡張領域を1つ前にずらします。
@sanomari
Copy link
Contributor Author

sanomari commented Jun 2, 2022

せっかく指摘をいただいたので、対応のレベルを少し上げます。

  • レベル1 不具合を解消させる(当初のPR)
  • レベル2 拡張領域の確保を必要な分だけに絞る ←いまここ
  • レベル3 拡張領域を使わないようにする
  • レベル4 CAppNodeGroupHandle::AddEditWndListの処理内容を見直す(恒久対応)

PR前の確認ではレベル3まで動作検証してます。
レベル4をやり出すと大変なので、このPRは「課題ありの暫定対応」のままマージするイメージでした。

@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell E 13 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@AppVeyorBot
Copy link

Build sakura 1.0.4142 completed (commit b526697a4d by @sanomari)

@k-takata
Copy link
Member

k-takata commented Jun 2, 2022

  • レベル2 拡張領域の確保を必要な分だけに絞る ←いまここ

いったんここまでマージしてしまうのがよさそうに思います。

Copy link
Member

@kengoide kengoide left a comment

Choose a reason for hiding this comment

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

master において不具合が発生すること、本PRで解消することを確認しました。

#1794 のレビューが甘かったようです。ご迷惑をおかけしました。

@sanomari sanomari merged commit b7538a1 into sakura-editor:master Jun 9, 2022
@sanomari sanomari deleted the workaround/issue1845 branch June 9, 2022 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug🦋 ■バグ修正(Something isn't working)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants