プログレスバーの表示切り替え方法を変更して切り替えが正しく反映されるようにする #1727

ghost · 2021-09-21T05:28:18Z

PR の目的

プログレスバーの表示/非表示を切り替える方法を変更し、切り替えが画面上に正しく反映されるようにする。

カテゴリ

仕様変更

PR の背景

#1601 にてステータスバーの描画処理が改善されましたが、「ファイル読み込み後にプログレスバーが表示されたままになる」問題が発生しています（ #1651 ）。

PR のメリット

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

仕様・動作説明

今回問題となっているプログレスバーはステータスバーの左端に表示されるもので、内部上はステータスバーの子ウィンドウになっています。どちらもCMainStatusBarクラスにウィンドウハンドルが保持されています。

ファイルの読み込み処理では、CDocFileOperationクラスからCDocSubjectを通じて、各クラスの前処理（OnBeforeLoad）・中処理（OnLoad）・後処理（OnAfterLoad）が呼び出されます。CVisualProgressクラスもそうして呼び出されるクラスの一つで、前処理と後処理の段階ではプログレスバーの表示/非表示の切り替えを行っています。
また、CEditViewクラスも後処理でプログレスバーの表示が切り替えられた後に呼び出されており、ここで範囲選択の解除、カーソルの移動、ステータスバーの更新などを実行しています。

プログレスバーはステータスバー上に表示される以上、表示の切り替えを行うとステータスバー上の領域を再描画する必要が生じるはずですが、表示の切り替えを行う各関数には再描画を実施する命令が記述されていません。

このため、以前はステータスバーや編集ウィンドウが描画されるタイミングで再描画されていた領域が、 #1601 で描画範囲とタイミングが調整されたために範囲外となって再描画されなくなったものと推定しました。

そこで、CMainStatusBarクラスにプログレスバーの表示を切り替え、ステータスバー上の表示領域を更新するメソッドを新設し、これをCVisualProgressから利用するように変更します。

なお、既存コードにおける変更対象であるCVisualProgressはファイル保存の過程でも利用されるため、この変更はファイル保存時のプログレスバーの取り扱いにも影響します。一方、CVisualProgress以外でプログレスバーの表示切り替えを行う各所のコードは変更していません。

PR の影響範囲

ファイル読み込み処理におけるプログレスバーの表示
ファイル保存処理におけるプログレスバーの表示

テスト内容

ファイルのロード中、プログレスバーが表示されること。
ファイルのロード完了後、プログレスバーが表示されないこと。
ファイルの保存中、プログレスバーが表示されること。
ファイルの保存完了後、プログレスバーが表示されないこと。

参考資料

AppVeyorBot · 2021-09-21T05:57:59Z

✅ Build sakura 1.0.3913 completed (commit ca69558ccf by @kazasaku)

sakura_core/window/CMainStatusBar.cpp

berryzplus · 2021-09-21T10:32:31Z

sakura_core/window/CMainStatusBar.cpp

+	@breif プログレスバーを表示する
+*/
+void CMainStatusBar::ShowProgressBar() const {
+	if (m_hwndStatusBar && m_hwndProgressBar) {


指摘（≒修正要求的な何か）ではないですが、条件式が若干悩ましいと思いました。

「ステータスバーがあるとき（ハンドルが0以外）、かつ、プログレスバーがあるとき（ハンドルが0以外）」

・・・つまり、プログレスバーが無かったらこのif文には入らないんですよね。
「Showする」ってことは「見えてない」んですけど、存在してなければならないっていう縛り。

「見えてないなら存在しないのと同じ」と言えるかどうかは少し微妙なんですが、
非表示のプログレスバーになにか使い道があるようには思えないので、
必要なときだけ作成するアプローチに変えてもいいんじゃないかなと思います。

今の実装だと、ステータスバーとプログレスバーは同時に作成され同時に破棄されるように見えます。
（CMainStatusBar以外に作成・破棄をする箇所はなさそうです。）
したがって「見えてはいないが存在する」ことになるのでこの条件にしています。

不可視のプログレスバーに存在意義があるのかは僕も疑問を感じるので、プログレスバーの表示切り替え時に作成・破棄するようにして、ShowProgressBarは作成したプログレスバーのハンドルを返すようにしても良いかもしれません。

berryzplus · 2021-09-21T10:35:29Z

sakura_core/window/CMainStatusBar.h

@@ -56,6 +56,8 @@ class CMainStatusBar : public CDocListenerEx{

 	//設定
 	bool SetStatusText(int nIndex, int nOption, const WCHAR* pszText, size_t textLen = SIZE_MAX);
+	void ShowProgressBar() const;	// プログレスバーを表示する
+	void HideProgressBar() const;	// プログレスバーを非表示にする


これも指摘じゃないです。

引数なしの関数に ( void ) って付ける人と付けない人がいますね。
付けないメリットは、タイプする文字が少なくて済むこと。
付けるメリットは不明です。（何かあるのか？古代のコンパイラが処理できなかったという歴史的背景以外に）

C言語のコンパイラによっては、関数のプロトタイプ宣言と異なる呼び方をしてもコンパイルやリンクが通ってしまう場合があります。ヘッダファイルを #include し忘れて関数のプロトタイプ宣言をちゃんと引っ張っていない場合とかですけれど。

で、何やら引数無しの関数のプロトタイプ宣言で void を付けておかないと、検査してくれないようです。

https://www.jpcert.or.jp/sc-rules/c-dcl20-c.html

https://rules.sonarsource.com/c/RSPEC-929

C++ の場合はこの問題は無いと思うので引数無しの関数やメソッドには void は付ける必要は無いと思いますが、別に付いてても実害は無いです。SB創業者的に不毛ですがコーディング標準で求められる事はあります。

berryzplus · 2021-09-21T10:39:08Z

sakura_core/window/CMainStatusBar.h

@@ -56,6 +56,8 @@ class CMainStatusBar : public CDocListenerEx{

 	//設定
 	bool SetStatusText(int nIndex, int nOption, const WCHAR* pszText, size_t textLen = SIZE_MAX);
+	void ShowProgressBar() const;	// プログレスバーを表示する


書きっぷりの提案（あまり重要ではない）

Suggested change

void ShowProgressBar() const; // プログレスバーを表示する

void ShowProgressBar() const;

👆見たらわかるからコメント要らなくね？とか。

Suggested change

void ShowProgressBar() const; // プログレスバーを表示する

void ShowProgressBar() const; //!< プログレスバーを表示する

👆せっかくコメント付けるのだから、doxygenコメントにしよう、とか。

beru · 2021-09-21T23:40:53Z

表示する ShowProgressBar と非表示にする HideProgressBar でメソッドを分けると条件によって呼び出しをする記述を別にしないといけないので、1つのメソッドの bool 型の引数で表示/非表示を切り替えられるようにした方が良いと思います。

AppVeyorBot · 2021-09-21T23:53:05Z

✅ Build sakura 1.0.3914 completed (commit e98b3415ab by @kazasaku)

ghost · 2021-09-22T15:09:58Z

表示する ShowProgressBar と非表示にする HideProgressBar でメソッドを分けると条件によって呼び出しをする記述を別にしないといけないので、1つのメソッドの bool 型の引数で表示/非表示を切り替えられるようにした方が良いと思います。

CVisualProgress以外の既存コードだと、同じ関数でプログレスバーの表示と非表示を切り替えている箇所もあるので、それらを置き換えていくことを考えると、::ShowWindow()と同様に引数で切り替えたほうが良さそうですね。
お手数おかけします。

sonarcloud · 2021-09-22T15:29:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

AppVeyorBot · 2021-09-22T15:43:19Z

✅ Build sakura 1.0.3915 completed (commit 34eb9b1c10 by @kazasaku)

beru

不具合が解消された事を確認しました。
問題無いと思います。

berryzplus · 2021-09-22T22:53:22Z

sakura_core/window/CMainStatusBar.h

@@ -56,6 +56,7 @@ class CMainStatusBar : public CDocListenerEx{

 	//設定
 	bool SetStatusText(int nIndex, int nOption, const WCHAR* pszText, size_t textLen = SIZE_MAX);
+	void ShowProgressBar(bool bShow) const;


対処しなくてよいですが、const付けたらいけなくないですかね。

・const付けたメソッド　＝　クラスが表現する何かの状態を変えないメソッド。
・const付けないメソッド　＝　クラスが表現する何かの状態を変更するメソッド。

プログレスバーを表示するか否かは、「サクラエディタのメインステータスバー」の状態だと思います。
クラスが表現する何か（＝サクラエディタのメインステータスバー）の状態（＝プログレスバーを表示するか否か）を変えるメソッドだから、const付けたらいかんような。

メンバ変数を書き換えないので付けたほうがいいかな？と思っていました。

ここらへん微妙ですよね。handle 経由で状態を変更しているので付けない方が良いのか、それともメンバ変数の値は変更していないので付けたほうが良いのか。

ちょっと別のケースですが、引数に const を付けた方が可読性が上がるのに付けないコードをよく見かけます（どこで？）。モーなんだかconst教とかいう宗教法人作って節税して選挙に当選して法律でconst付加を義務付けてほしいぐらいです。

メンバ変数を書き換えないので付けたほうがいいかな？と思っていました。

はい、その感覚は正しいと思います。

自分が言ってたのは逆で、メソッドの意味合い的に非constなメソッドだから、
メンバ変数を書き替えないといかんのじゃないか？
ってことです。

修正量多くなりますし、対応する必要はないと思うんですが。

berryzplus · 2021-09-22T22:59:55Z

sakura_core/window/CMainStatusBar.h

@@ -56,6 +56,7 @@ class CMainStatusBar : public CDocListenerEx{

 	//設定
 	bool SetStatusText(int nIndex, int nOption, const WCHAR* pszText, size_t textLen = SIZE_MAX);
+	void ShowProgressBar(bool bShow) const;


これも指摘じゃないですが、こんな考え方もあります。

Suggested change

void ShowProgressBar(bool bShow) const;

void ShowProgressBar(bool bShow = true) const;

こうしておくと、表示するとき「引数なし」で呼び出せるようになります。
「表示する」がメイン機能で、「引数falseを与えれば非表示にもできる」になります。

メリットは「ShowProgressbar(true);のtrueってなんじゃ～？」を確認する必要がなくなることです。

ghost · 2021-09-23T16:15:47Z

レビューありがとうございます。マージします。
他に不都合あれば対処しますのでお知らせください。

プログレスバーの表示切り替え方法を変更

efe00be

berryzplus reviewed Sep 21, 2021

View reviewed changes

コメントの削除

fd450ba

beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Sep 21, 2021

berryzplus previously approved these changes Sep 22, 2021

View reviewed changes

メソッドを一つにまとめ、引数値で切り替えるように変更

13ee731

ghost dismissed berryzplus’s stale review via 13ee731 September 22, 2021 15:13

beru approved these changes Sep 22, 2021

View reviewed changes

berryzplus reviewed Sep 22, 2021

View reviewed changes

berryzplus approved these changes Sep 22, 2021

View reviewed changes

ghost merged commit 14335fd into sakura-editor:master Sep 23, 2021

ghost deleted the feature/change_show_progressbar branch September 23, 2021 16:17

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

プログレスバーの表示切り替え方法を変更して切り替えが正しく反映されるようにする #1727

プログレスバーの表示切り替え方法を変更して切り替えが正しく反映されるようにする #1727

ghost commented Sep 21, 2021

AppVeyorBot commented Sep 21, 2021

berryzplus Sep 21, 2021

ghost Sep 21, 2021

berryzplus Sep 21, 2021

beru Sep 22, 2021

berryzplus Sep 21, 2021

beru commented Sep 21, 2021

AppVeyorBot commented Sep 21, 2021

ghost commented Sep 22, 2021 •

edited by ghost

Loading

sonarcloud bot commented Sep 22, 2021

AppVeyorBot commented Sep 22, 2021

beru left a comment

berryzplus Sep 22, 2021

ghost Sep 23, 2021

beru Sep 23, 2021

berryzplus Sep 24, 2021

berryzplus Sep 22, 2021

ghost commented Sep 23, 2021

	void ShowProgressBar() const; // プログレスバーを表示する
	void ShowProgressBar() const;

	void ShowProgressBar() const; // プログレスバーを表示する
	void ShowProgressBar() const; //!< プログレスバーを表示する

	void ShowProgressBar(bool bShow) const;
	void ShowProgressBar(bool bShow = true) const;

プログレスバーの表示切り替え方法を変更して切り替えが正しく反映されるようにする #1727

プログレスバーの表示切り替え方法を変更して切り替えが正しく反映されるようにする #1727

Conversation

ghost commented Sep 21, 2021

PR の目的

カテゴリ

PR の背景

PR のメリット

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

仕様・動作説明

PR の影響範囲

テスト内容

関連 issue, PR

参考資料

AppVeyorBot commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beru commented Sep 21, 2021

AppVeyorBot commented Sep 21, 2021

ghost commented Sep 22, 2021 • edited by ghost Loading

sonarcloud bot commented Sep 22, 2021

AppVeyorBot commented Sep 22, 2021

beru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 23, 2021

ghost commented Sep 22, 2021 •

edited by ghost

Loading