HashidaTKS commented 9 months ago

Which issue(s) this PR fixes:

https://github.com/ThinBridge/Chronos-SG/issues/119

What this PR does / why we need it:

Set CEF's external_message_pump setting TRUE.

This patch fixes a bug that shortcut keys like "Ctrl + N (new tab)" don't work on Windows 11. Also, this patch fixes a bug that a Chronos process is left after exiting on Windows 11.

https://cef-builds.spotifycdn.com/docs/119.1/structcef__settings__t.html#af11b432414c7002991e3b6ae9b2f1f07

This option is recommended for use in combination with the CefDoMessageLoopWork() function in cases where the CEF message loop must be integrated into an existing application message loop

This is the case for us.

How to verify the fixed issue:

The steps to verify:

Start CEF on Windows 10/11
Execute shortcut keys
- Ctrl + N, T, P, W, Tab,1～9
Exit Chronos

Expected result:

The shortcut keys works fine on Windows 10 and 11.
Chronos processes are not left after exiting.
Display "Chronos System Guardを終了してもよろしいですか？" dialog when exiting if SG mode

HashidaTKS commented 9 months ago

メモ:

Windows 11で、右上のxボタンなどからChronosを終了しても、Chronosのプロセスが残留する問題が起きていた（確かに手元でも発生していたのだが、問題だと気付いていなかった...）。説明欄に記載の通り、この修正によりその問題も直っている様子。

external_message_pumpをtrueにしたことにより、アプリケーション独自のメッセージがCEFにも伝わるようになった（という認識な）ので、上記の問題が直るのも納得がいく。

kenhys commented 9 months ago

Windows 11でショートカットキーが効かない問題が解消されている。
しかし、Chronos SG終了時にNoを選択して再起動させるということができなくなっている。(なにも確認されない)

HashidaTKS commented 9 months ago

確かに...。

HashidaTKS commented 9 months ago

https://github.com/ThinBridge/Chronos/blob/master/Sazabi.cpp#L1644

上記の;;MessageBoxを通っているが、メッセージボックスが表示されずにIDYESが返ってくる。終了しようとしていることを理解して、ダイアログを表示しないようになってしまっているのだろうか...。

HashidaTKS commented 9 months ago

https://github.com/ThinBridge/Chronos/blob/master/MainFrm.cpp#L353 https://github.com/ThinBridge/Chronos/blob/master/Sazabi.cpp#L140

同様にMessageBox関係でHWNDにNULLを指定している場所は上記の場所があったが、これらの場所では問題なく動作している。問題が発生するのはExitするときだけの模様。

HashidaTKS commented 9 months ago

動作自体は確かに期待通りになる。 MB_DEFAULT_DESKTOP_ONLYをなぜ追加したかについての説明をSazabi.cppのコードのコメントとしていれておいたほうがよさそうである。

ありがとうございます。コメントを追加しました。また、コミットを一つにまとめました。

HashidaTKS commented 9 months ago

https://www.codeproject.com/Articles/35209/MessageBox-in-ExitInstance

WM_QUITがメッセージキューの中にいると、新しいダイアログが作成されないというMFCの仕様とのこと。 ExitInstance関数の中がまさにそのような状況で、今回のケースに該当している。 external_message_pumpをtrueにしたことで、メッセージの伝達順序が変わった等でこの状況になるようになったのだろう（恐らく変更前の状態がMFC的におかしいのではないか）。

MB_DEFAULT_DESKTOP_ONLYやMB_SERVICE_NOTIFICATIONが指定された場合、現在のウィンドウに紐づかないと解釈されて、現在のウィンドウのWM_QUITも無視して動くのかなぁ。

上記の例のようにWM_QUITをメッセージキューから除外する方がよいのだろうか。 WM_QUITを読み飛ばすようにすることでもダイアログが表示されるようになることは確認済み。

ashie commented 9 months ago

間違って https://github.com/ThinBridge/Chronos-SG/issues/119 に書いてしまったので転載。

cef_settings_t.multi_threaded_message_loopの説明に書かれているCefBrowserProcessHandler::OnScheduleMessagePumpWork()を実装していないのが気になるところです。

あまり実装例が見つからないのですが、CefSharpの例はこんな感じでした

https://github.com/cefsharp/CefSharp/blob/ed1763ee9510dca9e54ab766ea7cf305f71d2aa2/CefSharp.Wpf.Example/Handlers/WpfBrowserProcessHandler.cs#L47-L63
        protected override void OnScheduleMessagePumpWork(long delay)
        {
            //If the delay is greater than the Maximum then use ThirtyTimesPerSecond
            //instead - we do this to achieve a minimum number of FPS
            if (delay > ThirtyTimesPerSecond)
            {
                delay = ThirtyTimesPerSecond;
            }

            //When delay <= 0 we'll execute Cef.DoMessageLoopWork immediately
            // if it's greater than we'll just let the Timer which fires 30 times per second
            // care of the call
            if (delay <= 0)
            {
                dispatcher.BeginInvoke((Action)(() => Cef.DoMessageLoopWork()), DispatcherPriority.Normal);
            }
        }
Chronosでやるとしたら、正の値のdelayが来たらその時間だけCefDoMessageLoopWork()の呼び出しをスキップする？これを実装することでMessageBoxの件が改善するのかしないのか、気になるところです。

上記の

Chronosでやるとしたら、正の値のdelayが来たらその時間だけCefDoMessageLoopWork()の呼び出しをスキップする？これを実装することでMessageBoxの件が改善するのかしないのか、気になるところです。

はちょっと違うかなぁと後になって思った。 CefSharpと似たような感じにすべきか。メッセージループのところを抜本的に見直した方が良いかもしれない。

ただ、今回のリリースでそこまでやるのは無理なので、

すごく黒魔術感あるので @ashie さんの判断も待ちたい。

現状ではすぐには判断つかないなぁと思ってたのですが、ある程度メカニズムが解明されて、通しテストの結果が良好なのであれば応急処置としては現状でマージしても良いのかなと思い始めてます。

いずれにしてもメッセージループの根本的なところに手を入れるので、他の箇所で思わぬデグレが発生する可能性もあると思います。なので入念に通しテストをした方が良さそうです。

HashidaTKS commented 9 months ago

ありがとうございます。 OnScheduleMessagePumpWorkは既存の存在するものが呼び出されるというような解釈をしていたのですが、自分で実装しなければいけないのですね。。。ちゃんと確認すべきでした。

kenhys commented 9 months ago

RC4に投入することになった。

ThinBridge / Chronos

Fix a bug that some messages are not handled on Windows11 #151

Which issue(s) this PR fixes:

What this PR does / why we need it:

How to verify the fixed issue:

The steps to verify:

Expected result: