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

ターゲットwindowsをwindows7に上げる #548

Merged

Conversation

berryzplus
Copy link
Contributor

ターゲットwindowsのバージョンをwin2000からwin7に上げます。

副産物として以下の変更を同時に行います。

  • 古いSDKによるビルドで必要だった windows SDK 定数の独自定義の除去
  • windows vistaまでのosバージョン確認に使用していた独自クラス(COsVersionInfo)の除去
  • GetProcAddressしなくて良くなった関数呼出を標準の記載に戻す
  • HtmlHelp関数(API)を呼び出すためのクラスの処理が他実装と乖離していたのを合わせる

この変更の統合により、修正が必要と判断したコードをよく調べてみたらデッドコードだった、という悲劇に遭遇する確立を少しだけ下げることが出来ます。

プロジェクト設定が widows 2000 向けになっていたのを win7 向けに更新します。
ここだけ COsVersionInfoクラス を使っていたのでグローバル関数を呼ぶように修正します。
処理を簡単に記述するために暗黙動的リンクを採用するので、
動作対象外OSではそもそも起動できなくなったため、チェックをする意味がなくなりました。
シグニチャ訂正をすると他に影響が出るので、一旦内部を空っぽにする対処を行います。
実際に利用されている関数を残して未使用処理を除去。
@berryzplus berryzplus force-pushed the feature/upgrade_target_winver branch from bdaaae5 to 05f6df4 Compare October 13, 2018 15:39
OPENFILENAMEZの廃止により、利用していた箇所は除去済み。
未使用関数のため影響なし。
IsWine関数は実際には「プロセス毎にキーボードアクセラレータを作る必要があるか?」の判定に使われていた。
アクセラレータはプロセス毎に管理するものなので共有しないほうが正しいように思われる。
判定文を除去し、常に自プロセスのアクセラレータテーブルを作成するように変更する。
対象OSをwin7に上げたことで不要になったGetProcAddressを除去します。
RAIIクラスを使ったパス移動は、無名スコープを抜ける際に解除されるので意味がありません。
カレントディレクトリに変更は比較的重たいので無駄と分かって残しておく意味はないと思われます。
他のCDllImp派生クラスと実装が異なっていた。
HtmlHelpはwindows SDKに定義がある特殊な外部関数なので、
windows SDKの定義とCDllImp実装の両方の特性を活かせるように実装し直します。
単にチェックの有無で倍率設定を切り替える処理となっている。
Dx配列の導出用にstd::vectorを使っていたが、ここの処理では文字列長が固定なので動的サイズ変更しなくてよかった。
@berryzplus berryzplus added enhancement ■機能追加 refactoring リファクタリング 【ChangeLog除外】 labels Oct 14, 2018
@berryzplus berryzplus added this to the next release milestone Oct 14, 2018
グリフインデックスの受取バッファにstd::wstringを使っていたが、stringの機能は全く使わないので単なるバッファに変更する。
確保量が毎回同じで、かつ、小さいのでスタックに積んで動的確保のコストを削る。
}
#endif
// Windowsバージョンは廃止。
// 動作可能バージョン(=windows7以降)でなければ起動できない。
Copy link
Contributor

Choose a reason for hiding this comment

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

黒翼猫さんが Windows 2000 で無理やり動作させた時は無力感を味わいながら諦めましょう…。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

大変お世話になってた気がするので、もし必要なら対応版ブランチを作る感じです。

きっと彼らは最新版追わないので影響ないんじゃないかと思ってます。

@@ -844,7 +843,7 @@ void CMenuDrawer::MyAppendMenu(
// メニュー項目をオーナー描画にして、アイコンを表示する
// 2010.03.29 アクセスキーの分を詰めるためいつもオーナードローにする。ただしVista未満限定
// Vista以上ではメニューもテーマが適用されるので、オーナードローにすると見た目がXP風になってしまう。
if( m_pShareData->m_Common.m_sWindow.m_bMenuIcon || !IsWinVista_or_later() ){
if( m_pShareData->m_Common.m_sWindow.m_bMenuIcon ){
Copy link
Contributor

Choose a reason for hiding this comment

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

PR の変更内容への指摘では無くて余談ですが、もうメニューをオーナードローでやらなくても良いかも知れないですね。いやまぁ確認して Issue 立てろと言われたらそれまでですが…。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

メニューのオーナードローをやめる対応は後回しにしました。

これマージした後に ‘#if’ で無効化されてるコードを除去するのをやろうとしていて、それの後くらいがいいかなと思っています。

メニュー処理はコピペで二重化されてるので、整理して一本化するのが先かもです。

Copy link
Contributor

@beru beru left a comment

Choose a reason for hiding this comment

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

メニューバーにメッセージを表示する処理の変更内容が他と比べてちょっと異質な感じはありますが問題無いと思います。

確認していませんが、エディタ側の Unicode 文字列描画処理は等幅専用だったりして使えないのかもしれないですね。それか再利用可能な形になっていないとか。

@m-tmatma
Copy link
Member

ターゲットwindowsのバージョンをwin2000からwin7に上げます。

refactoring のタグは違うように思います。どちらかというと仕様変更だと思います。
気持ちは refactoring なのはわかります。

@berryzplus
Copy link
Contributor Author

確認ありがとうございます。
マージします。

@berryzplus berryzplus merged commit 51d5907 into sakura-editor:master Oct 15, 2018
@berryzplus
Copy link
Contributor Author

refactoring のタグは違うように思います。どちらかというと仕様変更だと思います。
気持ちは refactoring なのはわかります。

あとでタグ外しときます。

@ds14050
Copy link
Contributor

ds14050 commented Oct 15, 2018

Windows Vista での起動確認を報告します。すでにビルドはできませんが動作テストはまだできるようです。

@beru beru removed the refactoring リファクタリング 【ChangeLog除外】 label Oct 15, 2018
@berryzplus
Copy link
Contributor Author

Windows Vista での起動確認を報告します。
すでにビルドはできませんが動作テストはまだできるようです。

いまのところ、vistaで使えなくなるような変更は入ってないので、起動はできる想定でした。
当面は vista は「意識しない」というだけで「使えるはず」な状態が続くと思ってます。

@berryzplus berryzplus deleted the feature/upgrade_target_winver branch October 15, 2018 14:48
@ds14050
Copy link
Contributor

ds14050 commented Oct 17, 2018

コミット 05f6df4 からだと思いますが、MinGW ビルドがこけています。

extmodule/CHtmlHelp.cpp: In member function 'virtual bool CHtmlHelp::InitDllImp()':
extmodule/CHtmlHelp.cpp:53:5: error: invalid conversion from 'CHtmlHelp::FnPtr_HtmlHelp' {aka 'HWND__* (*)(HWND__*, const wchar_t*, unsigned int, long long unsigned int)'} to 'void*' [-fpermissive]
   { m_pfnHtmlHelp,  "HtmlHelpW" },
     ^~~~~~~~~~~~~
mingw32-make: *** [Makefile:448: extmodule/CHtmlHelp.o] Error 1

@berryzplus
Copy link
Contributor Author

MinGW ビルドがこけています。

こういうのがあるから appveyor に組み込んでおきたかったんです(キリッ

・・・調べて対応します 😭

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…target_winver

ターゲットwindowsをwindows7に上げる
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ■機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants