sunfish-shogi / shogihome

PC で動く高機能な将棋の GUI「ShogiHome」の開発リポジトリ
https://sunfish-shogi.github.io/shogihome/
MIT License
138 stars 20 forks source link

Force Reloading While Locale Change Functionality #488

Open Quisette opened 1 year ago

Quisette commented 1 year ago

Description

When changing locales, a full restart is required to apply the locale settings, which makes locale changing more complex.

I've come up with some ideas to resolve this problem:

Comment below If you have any thoughts, thanks!

sunfish-shogi commented 1 year ago

@Quisette As you said, we should bother do save, close, and relaunch. Actualy, I often do restart operation to change locale.

But it is for only checking layout. I think most of users do only once.

And we need to think about relation any app states (USI engine, CSA game session, and more), because user can always open app setting dialog.

Quisette commented 1 year ago

@sunfish-shogi

As you said, we should bother do save, close, and relaunch. Actualy, I often do restart operation to change locale. But it is for only checking layout. I think most of users do only once.

Absolutely. Looks this problem is not the primary problem to solve.

And we need to think about relation any app states (USI engine, CSA game session, and more), because user can always open app setting dialog.

Can you elaborate more about this? Or create a new issue separately to talk about it more. Thanks!

sunfish-shogi commented 1 year ago

@Quisette

And we need to think about relation any app states (USI engine, CSA game session, and more), because user can always open app setting dialog.

Can you elaborate more about this?

Main process has TCP sessions of CSA protocol and child proccesses of USI engin. And these have session ID for communicate with renderer process.

スクリーンショット (89)

Doing reload only renderer, we shoud implement to clean up TCP sessions and child processes, and recreate menu bar.

Electron Shogi prevents shutdown during CSA online game, because some AI developers use on computer shogi championship and impotant experiments. In future, I may add more restriction of shutdown if need. Either way of renderer reload and completely restart, we need to care those app states.

As a side note, when I release new minor version, I create checklist and do many checkes for quality assesment. So if the feature has not high priolity , I want to keep behavior patterns to be few.

Quisette commented 1 year ago

@sunfish-shogi

Doing reload only renderer, we shoud implement to clean up TCP sessions and child processes, and recreate menu bar. Electron Shogi prevents shutdown during CSA online game, because some AI developers use on computer shogi championship and impotant experiments. In future, I may add more restriction of shutdown if need. Either way of renderer reload and completely restart, we need to care those app states.

Got it. Seems we need to have a full restart to refresh the electron app, and prevent webview restart when a game is started ( both locally and on internet). For those who want to force restart the client, we should also offer a full detachment of TCP sessions and processes.

As a side note, when I release new minor version, I create checklist and do many checkes for quality assesment. So if the feature has not high priolity , I want to keep behavior patterns to be few.

Seems that you are talking the recent PR (#492) and you tend to add one or two feature per minor version update. Yeah, this feature can be slightly postponed if there is already one or two features on the list of upgrade, and the upgrade after this may contain this PR.

sunfish-shogi commented 1 year ago

As a side note, when I release new minor version, I create checklist and do many checkes for quality assesment. So if the feature has not high priolity , I want to keep behavior patterns to be few.

Seems that you are talking the recent PR (https://github.com/sunfish-shogi/electron-shogi/pull/492) and you tend to add one or two feature per minor version update. Yeah, this feature can be slightly postponed if there is already one or two features on the list of upgrade, and the upgrade after this may contain this PR.

Sorry, I talked about increase of regression test pattern. I always update checklist of all functions(already over 250 items) by each minor version updates as screenshot below. And I check existed features which not checked recently and new features.

スクリーンショット (90)

sunfish-shogi commented 1 year ago

By the way, we can do reload by developer tools window ( using (Shift +) Ctrl/Cmd + R ) . It is very faster than full restart.

And I tried to use app.relaunch and app.exit as following code.

function createMenuTemplate() {
  const menuTemplate: Array<MenuItemConstructorOptions | MenuItem> = [
    {
      label: t.file,
      submenu: [
        {
          label: "restart",
          click: () => {
            app.relaunch();
            app.exit();
          },
        },

It works on release build, but running by npm run electron:serve command, it can not load content after relaunch.

スクリーンショット (91)

Quisette commented 1 year ago

Got it. Thanks for the hard work on regression test to maintain this project's stability and quality! :+1: It is OK to merge my PR any time after this wave of regression test has ended.

Quisette commented 1 year ago

By the way, we can do reload by developer tools window ( using (Shift +) Ctrl/Cmd + R ) . It is very faster than full restart.

That is the way I am restarting during writing translations.

And I tried to use app.relaunch and app.exit as following code. It works on release build, but running by npm run electron:serve command, it can not load content after relaunch.

The result is the same in Ubuntu using AppImages. During app.exit(), the npm run electron:serve process is exited then closed, hence the program cannot be reloaded. In electron:preview, the process quits as well, but the reloaded app is functional with renewed menu bar and translations.

Maybe we need to think that how to reload the menu only, or just notify devs that the reload works only in electron:preview and compiled .exe or .AppImage .