編集ウインドウへの参照をリファクタリングする #1744
編集ウインドウへの参照をリファクタリングする #1744
Conversation
CEditWndのインスタンスにはグローバル変数のようにアクセスできる。このため、コンストラクタでCEditWndのインスタンスを要求してクラス内外でのアクセスに流用する構造は無駄と考えられる。
置換パターン \b((m_)?pc?)?(Edit)?View(Dst)?->m_pcEditWnd->
\b((m_)?pc?)?(Edit)?Doc(Ref)?->m_pcEditWnd->
|
❌ Build sakura 1.0.3960 failed (commit 6404e24457 by @sanomari) |
|
修正内容に異論はないですが、テストが通らないのはマズいです。 |
CEditWndのインスタンスはクラスのstatic変数に保持されるので、グローバル変数に持つと二重保持になり無駄です。
sanomari
force-pushed
the
feature/fix_reference_of_CEditWnd
branch
from
a424ce5 to
4b341c4
Compare
|
SonarCloud Quality Gate failed. |
|
✅ Build sakura 1.0.3961 completed (commit 94adbd584e by @sanomari) |
|
SonarCloud対応をやりかけましたが、サイズボックスの整理をしてから対応したほうが分かりやすそうなのであえてすべて放置します。 |
| @@ -89,7 +89,7 @@ bool CAutoReloadAgent::_ToDoChecking() const | |||
| if(hwndActive!=CEditWnd::getInstance()->GetHwnd())return false; | |||
| if(!GetListeningDoc()->m_cDocFile.GetFilePathClass().IsValidPath())return false; | |||
| if(GetListeningDoc()->m_cDocFile.IsFileTimeZero()) return false; /* 現在編集中のファイルのタイムスタンプ */ | |||
| if(GetListeningDoc()->m_pcEditWnd->m_pPrintPreview ) return false; // 印刷プレビュー中 2013/5/8 Uchi | |||
| if(GetEditWnd().m_pPrintPreview ) return false; // 印刷プレビュー中 2013/5/8 Uchi | |||
There was a problem hiding this comment.
89行目にある CEditWnd::getInstance() も GetEditWnd() で置換できますね、とか。
92行目の m_pPrintPreview ) の空白がビミョーですね、とか。
突っ込もうとすればいくらでもツッコミどころがありそうです。
今回一応「基本的にパターン置換で対応」ということなので、細かいところはスルーします。
| @@ -173,9 +173,5 @@ struct SSearchOption{ | |||
| bool operator != (const SSearchOption& rhs) const noexcept; | |||
| }; | |||
|
|
|||
| //2007.10.02 kobake CEditWndのインスタンスへのポインタをここに保存しておく | |||
| class CEditWnd; | |||
| extern CEditWnd* g_pcEditWnd; | |||
| @@ -39,7 +39,7 @@ inline CEditDoc* CViewCommander::GetDocument() | |||
| } | |||
| inline CEditWnd* CViewCommander::GetEditWindow() | |||
| { | |||
| return m_pCommanderView->m_pcEditWnd; | |||
| return &GetEditWnd(); | |||
There was a problem hiding this comment.
このメソッド自体も、将来的には消えるべき、と理解しました。
今回は、分かりにくくなるからやらんのでしょうね・・・。
| @@ -1045,7 +1045,7 @@ bool IsFuncEnable( const CEditDoc* pcEditDoc, const DLLSHAREDATA* pShareData, EF | |||
|
|
|||
| // 02/06/26 ai Start | |||
| case F_JUMP_SRCHSTARTPOS: // 検索開始位置へ戻る | |||
| if( pcEditDoc->m_pcEditWnd->GetActiveView().m_ptSrchStartPos_PHY.BothNatural() ){ | |||
| if( GetEditWnd().GetActiveView().m_ptSrchStartPos_PHY.BothNatural() ){ | |||
There was a problem hiding this comment.
蛇足と思いますが、グローバル関数CEditView& GetActiveView()があったほうがよいと思います。
この使い方をしている箇所がけっこう多いから。
| // /* アクティブなペインを設定 */ | ||
| // m_pcEditWnd->SetActivePane( m_nMyIndex ); | ||
| // GetEditWnd().SetActivePane( m_nMyIndex ); | ||
| // } | ||
| // return 0L; |
There was a problem hiding this comment.
このコメントアウトコードは削ったら？
という指摘をしようかどうか迷いました。
とりあえず書いておきますが対応は不要です。
There was a problem hiding this comment.
この流れでCEditViewの修正も予定しています。
既に一括置換じゃない変更も含んでしまってますが、これは除外しておきたいです。
| @@ -851,7 +850,7 @@ LRESULT CEditView::DispatchEvent( | |||
| // マウスクリックによりバックグラウンドウィンドウがアクティベートされた | |||
| // 2007.10.08 genta オプション追加 | |||
| if( GetDllShareData().m_Common.m_sGeneral.m_bNoCaretMoveByActivation && | |||
| (! m_pcEditWnd->IsActiveApp())) | |||
| (! GetEditWnd().IsActiveApp())) | |||
There was a problem hiding this comment.
指摘じゃないですが、否定演算子の位置がおかしいですね。
There was a problem hiding this comment.
おかしいですね。
&&の後のカッコも不要という指摘もありえますよね。
面倒なので考えないことにしました。
| { | ||
| ::_com_raise_error(E_FAIL, MakeMsgError(L"Any CEditWnd has been instantiated.")); | ||
| } | ||
| return (CEditWnd&)*pcEditWnd; |
There was a problem hiding this comment.
const autoをC style castでconst外ししているため、キャストで強制型変換が可能になっており、もしCEditWnd::getInstance()の部分のコードが、CEditView*とか予期せぬ変な型に変更されたとしても、エラーが検出されなくて、この書き方は若干危険な予感がします。
非constを返すことが確定している以上、pcEditWndはconstで受けずautoだけにしてキャストを削除するか、const_castを使ったほうが安全です。ただconst_castはどうしても取り回しが難しいときに限定的に使うもので、こういうときは前者の対応方法をしたほうがいいと思います。
There was a problem hiding this comment.
了解しました。
この部分はissue化して別途対応します。
|
やや課題残ししてますがapproveいただいたのでマージしてしまいます。 |
PR の目的
編集ウインドウへの参照を持つクラスから参照メンバを除去し、編集ウインドウがインスタンス生成済みでなくても該当クラスのインスタンスを生成できるようにします。これにより、編集ウインドウは該当クラスをメンバ変数として持つ子ゴアできるようになります。
カテゴリ
PR の背景
サイズボックスの表示・非表示を切り替える修正を準備しています。
修正の過程で #1507 でも提案されているミニマップの分割を検討してみました。
https://github.com/sanomari/sakura-editor/tree/feature/add_cminimapview
現在の編集ビューはインスタンス生成に編集ウインドウのインスタンスを必要とします。
ミニマップは論理的に編集ウインドウのメンバですが、
編集ビューのコードを流用することで描画を実現しているので、編集ビューを継承するクラスとせざるを得ません。
しかし、編集ビューを継承させてしまうとインスタンス生成に編集ウインドウのインスタンスが必要となり、編集ウインドウのメンバとすることができません。
PR のメリット
PR のデメリット (トレードオフとかあれば)
仕様・動作説明
仕様変更・機能追加はありません。
編集ウインドウのポインタをメンバに持つクラスから該当メンバを削除します。
変更により、該当クラスのインスタンスを「編集ウインドウのメンバ」として保持できるようになります。
ミスすると大変なので、修正は可能な限り正規表現のパターン置換で行いました。
PR の影響範囲
サクラエディタのコア部分に関する変更です。
何かミスっていた場合、ビルドが通らなくなったりアプリ起動できなくなったりします。
テスト内容
ソースがビルドできること、ビルドしたアプリが起動できることを確認しました。
テスト1
手順
関連 issue, PR
参考資料