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.

Sign up for GitHub

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

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

Merged
3 commits merged into from Sep 23, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efe00be
プログレスバーの表示切り替え方法を変更
Sep 20, 2021
fd450ba
コメントの削除
Sep 21, 2021
13ee731
メソッドを一つにまとめ、引数値で切り替えるように変更
Sep 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sakura_core/uiparts/CVisualProgress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ void CVisualProgress::_Begin()
//プログレスバー
HWND hwndProgress = CEditWnd::getInstance()->m_cStatusBar.GetProgressHwnd();
if( hwndProgress ){
::ShowWindow( hwndProgress, SW_SHOW );
CEditWnd::getInstance()->m_cStatusBar.ShowProgressBar(true);
//範囲設定・リセット
Progress_SetRange( hwndProgress, 0, 101 );
Progress_SetPos( hwndProgress, 0);
Expand All @@ -121,7 +121,7 @@ void CVisualProgress::_End()
HWND hwndProgress = CEditWnd::getInstance()->m_cStatusBar.GetProgressHwnd();
if( hwndProgress ){
Progress_SetPos( hwndProgress, 0);
::ShowWindow( hwndProgress, SW_HIDE );
CEditWnd::getInstance()->m_cStatusBar.ShowProgressBar(false);
}

//砂時計
Expand Down
17 changes: 17 additions & 0 deletions sakura_core/window/CMainStatusBar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,20 @@ bool CMainStatusBar::SetStatusText(int nIndex, int nOption, const WCHAR* pszText
}
return bDraw;
}

//! プログレスバーの表示/非表示を切り替える
void CMainStatusBar::ShowProgressBar(bool bShow) const {
if (m_hwndStatusBar && m_hwndProgressBar) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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

// プログレスバーを表示するステータスバー上の領域を取得
RECT rcProgressArea = {};
ApiWrap::StatusBar_GetRect(m_hwndStatusBar, 0, &rcProgressArea);
if (bShow) {
::ShowWindow(m_hwndProgressBar, SW_SHOW);
} else {
::ShowWindow(m_hwndProgressBar, SW_HIDE);
}
// プログレスバー表示領域を再描画
::InvalidateRect(m_hwndStatusBar, &rcProgressArea, TRUE);
::UpdateWindow(m_hwndStatusBar);
}
}
1 change: 1 addition & 0 deletions sakura_core/window/CMainStatusBar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

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

private:
CEditWnd* m_pOwner;
HWND m_hwndStatusBar;
Expand Down