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

[WIP] ダイアログの位置が表示するたびにずれていく問題を修正 #687

Closed
wants to merge 2 commits into from

Conversation

beru
Copy link
Contributor

@beru beru commented Dec 8, 2018

Issue #397 に対処しました。

rc.left = rcWork.left;
if( rc.left < 0 ){
rc.right += rc.left;
rc.left = 0;
}
Copy link
Contributor Author

@beru beru Dec 8, 2018

Choose a reason for hiding this comment

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

ワークスペース座標系を考慮した処理に変更しましたが、モニタ1台の構成でしか動作確認していないので、プライマリモニタでないモニタ上でダイアログを閉じた場合の位置復帰がきちんと出来ていないかもしれないです。

Copy link
Member

Choose a reason for hiding this comment

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

PCのスペックに依存しますが、VirtualBoxで複数モニタ(最大8)の環境作れるっす。
ゲストOSは、MSサイトからダウンロードできるので、ご参考。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

情報ありがとうございます。後で確認してみます。
Windows10の仮想デスクトップでも問題が無いかの確認も必要ですね。

Copy link
Member

Choose a reason for hiding this comment

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

ワークスペース座標系を考慮した処理に変更しましたが、モニタ1台の構成でしか動作確認していないので、プライマリモニタでないモニタ上でダイアログを閉じた場合の位置復帰がきちんと出来ていないかもしれないです。

想定している手順等あれば、共有していただけますか?
手元にディスプレイ2つの環境があるので動作確認は可能です。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

操作手順

#397 (comment)

に書かれているようにタスクバーの位置を標準の下から左もしくは上に変えて試す必要があります。

なお複数ディスプレイの場合、プライマリモニタでないモニタをどこに配置するのかディスプレイ設定で?変えられると思うので、色々な配置で問題が無いか確認する必要があると思います。

対象となる画面

タイトル メニュー 実装クラス
ファイル内容比較 検索 > ファイル内容比較 CDlgCompare
DIFF差分表示 検索 > DIFF差分表示 CDlgDiff
履歴とお気に入りの管理 設定 > 履歴の管理 CDlgFavorite
ダイレクトタグジャンプ一覧 検索 > タグジャンプ CDlgTagJumpList
ウィンドウ一覧 ウィンドウ > ウィンドウ一覧表示 CDlgWindowList

@berryzplus
Copy link
Contributor

berryzplus commented Dec 9, 2018

PR ありがとうございます。

変更内容見ましたが、さすがにレベル高けぇな・・・と感じております。

構造としては CDlgDiff の問題に対処を試みて既定クラス CDialog に修正を行い、同様の問題を横展開するために OnMove を実装していたすべてのクラスを修正した、ということだと思います。

気になる点を以下に列挙します。

  1. 横展開してるはずの OnDestroy のシグニチャがブレてる
  2. CDialog::SetDialogPosSize()workWidthworkHeight は const にできます。
  3. OnSize 内で無駄に GetWindowRect を呼んでいる

1点目は指摘です。OnDestroy( void )OnDestroy() のどちらかに揃えたいです。
(どっちに揃えてもいいです)
2点目は感想です。あと width と height はこの順番で記述すべきです。
3点目は課題だと思っていて、別件対処要と考えています。

reviewコメントはあとでまとめて書きます。

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

指摘箇所4件です。
対応をお願いします。

sakura_core/dlg/CDialog.cpp Show resolved Hide resolved
sakura_core/dlg/CDialog.cpp Show resolved Hide resolved
m_yPos = rc.top;
m_nWidth = rc.right - rc.left;
m_nHeight = rc.bottom - rc.top;

Copy link
Contributor

Choose a reason for hiding this comment

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

メモ: 閉じるタイミングにだけ保存すれば開くときの復元に支障はないのでここで保存するのは無駄、と。

m_yPos = rc.top;
m_nWidth = rc.right - rc.left;
m_nHeight = rc.bottom - rc.top;

/* サイズボックスの移動 */
if( NULL != m_hwndSizeBox ){
::GetClientRect( m_hWnd, &rc );
Copy link
Contributor

Choose a reason for hiding this comment

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

メモ:この呼出しは無意味。WM_SIZE はクライアント矩形のサイズとともに送信されるから。

return TRUE;

Copy link
Contributor

Choose a reason for hiding this comment

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

メモ:既存コードの誤りなのでこのPRの話題として扱うべきではないが、ここの処理がこれでいいかとうかは結構ビミョー。
https://docs.microsoft.com/en-us/windows/desktop/winmsg/wm-move

BOOL CDlgCompare::OnSize( WPARAM wParam, LPARAM lParam )
{
/* 基底クラスメンバ */
CDialog::OnSize( wParam, lParam );

::GetWindowRect( GetHwnd(), &GetDllShareData().m_Common.m_sOthers.m_rcCompareDialog );

Copy link
Contributor

Choose a reason for hiding this comment

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

メモ:サイズ変更時から閉じる時に処理を移管したので、ここから消えるのは自然。

BOOL CDlgCompare::OnSize( WPARAM wParam, LPARAM lParam )
{
/* 基底クラスメンバ */
CDialog::OnSize( wParam, lParam );

::GetWindowRect( GetHwnd(), &GetDllShareData().m_Common.m_sOthers.m_rcCompareDialog );

RECT rc;
POINT ptNew;
::GetWindowRect( GetHwnd(), &rc );
Copy link
Contributor

Choose a reason for hiding this comment

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

メモ:無駄なAPI呼出。ON_SIZEメッセージはクライアント矩形のサイズとともに送られてくる。わざわざGetWIndowRectして矩形を取りなおす処理の必要性がそもそも疑問。というか、そもそもなんでwindowrectが必要なんだ?というツッコミもある。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ON_SIZE メッセージと書かれてますが正しくは WM_SIZE メッセージですね。
以上重箱の隅をつついてみました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

アンカー処理の実装(CDialog::ResizeItem)を読まないで適当なコメントを返しますが、GetWindowRect だとノンクライアント領域の分も含んでしまうので、コントロールの位置調整に使うのには適さない気もしますね。まぁでもノンクライアント領域が小さい場合には誤差みたいなものなのかも。。

Copy link
Contributor

Choose a reason for hiding this comment

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

ON_SIZE メッセージと書かれてますが正しくは WM_SIZE メッセージですね。

きゃーww

ダイアログには一応、枠がついてるので、枠を無視したサイズで子ウインドウの位置調整を行うのは危険です。というか、サイズを渡す意図なのにPOINT構造体使ってるのも気に食わんのです(笑

コメント、いっぱい書いてますけど本筋に関係ないものが多いのでスルーでもいいっすよ。対応が必要なものは「メモ:」って付けてないです。

{
::GetWindowRect( GetHwnd(), &GetDllShareData().m_Common.m_sOthers.m_rcCompareDialog );

return CDialog::OnMove( wParam, lParam );
Copy link
Contributor

Choose a reason for hiding this comment

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

メモ:これが消えたOnMoveの処理。

sakura_core/dlg/CDlgCompare.h Outdated Show resolved Hide resolved
sakura_core/dlg/CDlgDiff.h Outdated Show resolved Hide resolved
Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

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

間違った!指摘4件ですって。

@beru
Copy link
Contributor Author

beru commented Dec 9, 2018

@berryzplus さんレビューありがとうございます。指摘事項に対応しました。
ただ複数モニターでの確認とかしていないので、いったん [WIP] にします。

@beru beru changed the title ダイアログの表示位置が表示するたびにずれていく問題を修正 [WIP] ダイアログの表示位置が表示するたびにずれていく問題を修正 Dec 9, 2018
@beru beru added the 🐛bug🦋 ■バグ修正(Something isn't working) label Dec 9, 2018
@berryzplus
Copy link
Contributor

WIP状態のものをapproveするわけにはいかんので待ち・・・。

@beru
Copy link
Contributor Author

beru commented Dec 11, 2018

WIP状態のものをapproveするわけにはいかんので待ち・・・。

週末に複数モニタでの確認を行いますのでお待ちください。

@berryzplus berryzplus dismissed their stale review December 17, 2018 15:58

自分でdismissできるのに気付いたので change requestを取り下げます。

@beru beru changed the title [WIP] ダイアログの表示位置が表示するたびにずれていく問題を修正 [WIP] ダイアログの位置が表示するたびにずれていく問題を修正 Dec 25, 2018
@m-tmatma
Copy link
Member

#719 でコンフリクト発生しました。

@beru
Copy link
Contributor Author

beru commented Dec 25, 2018

#719 でコンフリクト発生しました。

連絡ありがとうございます。後で着手する際に rebase します。

@berryzplus
Copy link
Contributor

このPRは再着手待ちの認識です。

@beru beru self-assigned this Sep 5, 2020
@usagisita
Copy link
Contributor

僕はこういうWindowsの仕様に詳しくないので、レビューとか無理案件です。
でも毎回ウィンドウ位置がずれていって困ってるっぽいユーザーがいると思うと、涙が出ちゃいそうなんで、なんとかコミット、マージまで行ってほしいです。

そもそもこれってこういう認識でいいんでしたっけか
・ウィンドウ座標系だかは、マルチディスプレイでマイナス座標が使える
・ウィンドウ座標系と、タイトルバーと枠を抜いたクライアント座標系と、デスクトップのタスクバーを抜いた座標系があるっぽい
・タスクバーの座標系では上や左にタスクバーがあるとその分ウィンドウ座標系?とずれるので、ずれる
・タスクバー座標系は、WIndows XPだかVistaだかのときに、仕様が変更されたっぽいくて、ずれるアプリがある

頑張ってください。応援だけしています。

@k-takata
Copy link
Member

Vim でも同じようなバグがありました。vim-jp/issues#181
原因は Get/SetWindowPlacement() が扱う座標と他の API が扱う座標を混ぜて使っていたこと。
Get/SetWindowPlacement() が扱う座標は、タスクバーが上や左にあると、原点がその分下や右にずれるのですが、それ以外の API ではずれません。
(サクラが座標をどう使っているかは見ていません。)

@beru
Copy link
Contributor Author

beru commented Jul 24, 2021

PR作り直します。

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.

6 participants