Windows10メニュー対応 #130

KENCHjp · 2018-06-17T06:37:41Z

Windows10の場合に、sakura.exeのショートカットをグループ内ではなく、スタートメニュー直下に配置してアイコンを見つけやすくする対応。
#104

あわせて、バージョン情報を自動取得し、issファイルをリリース時に更新不要にする修正。

m-tmatma · 2018-06-17T07:23:59Z

この PR マージの前に、 Appveyor でビルドできる状態にしておきたいです。
レビュー自体はあらかじめやっておいていいですが。

KENCHjp · 2018-06-17T07:27:26Z

@m-tmatma さん、仰せの通りでお願いします。
GitHubとAppveyor廻りは私だと年が明けそう(辛)

m-tmatma · 2018-06-17T07:50:38Z

installer/sakura.iss

+begin
+  GetWindowsVersionEx(Version);
+  if (Version.Major = 10) and
+     (Version.Minor = 0) and


Minor バージョンのチェックは要りますか?

こちらの情報より、
http://yamatyuu.net/computer/program/sdk/base/GetVersionEx/index.html
それまでは、MajorバージョンはVista以降ずっと6だったようなので、
同じWindows10でもまたメニューの構造が変わらないとも限らないと思い、
現時点のバージョン限定の処理にしてみました。

同じWindows10でもまたメニューの構造が変わらないとも限らないと思い、
現時点のバージョン限定の処理にしてみました。

変わったときに対処するようにして、特に不都合がない限り
バージョン番号が変わっても問題とならないようにしたほうが
いいと思います。

Microsoft が　GetVersionExの戻り値を偽装するようにしたのは
必要がないのにバージョン番号を決め打ちしているアプリや
インストーラに対抗するためにしているのだと思います。

対応しました。

m-tmatma · 2018-06-17T07:51:54Z

installer/sakura.iss

+  GetWindowsVersionEx(Version);
+  if (Version.Major = 10) and
+     (Version.Minor = 0) and
+     (Version.ProductType = 1) then


ProductType は直値ではなく、VER_NT_WORKSTATION　等の値は使えないですか?
http://www.jrsoftware.org/ishelp/index.php?topic=isxfunc_getwindowsversionex

あと ProductType の値をチェックしている目的は何ですか?

VER_NT_WORKSTATIONに出来そうです。修正します。
ProductType は前のコメントと類似ですが今後の修正でメニュー構造が変わらないとも限らないとおもいかなりがちがちにしています。

m-tmatma · 2018-06-17T07:53:32Z

installer/sakura.iss

+  S: String;
+begin
+  GetWindowsVersionEx(Version);
+  if (Version.Major = 10) and


10 限定ですが、10 以上としたほうがいいように思います。

10以上という処理にするなら関数名は変えたほうがいいです。

こちらも前のコメントと同じで、10以上のUIが同じと限らないということで、
決め打ちとしています。
ご指摘３つは決めの問題かと思うので、どちらでもあまり思い入れはありません（修正はやぶさかではないです）

確かに11（は無いとはいわれてますが）でもおなじUIであれば、ここ修正する必要ありませんからね。

m-tmatma · 2018-06-17T07:54:41Z

installer/sakura.iss

+    Result := False;
+  end;
+end;
+function IsNotWin10 : Boolean;


IsNotWin10 は IsWin10を呼び出す形で実装できませんか?

こちら、確かにデッドコピーなので恰好悪いとは思ったのですが、
checkのパラメータにnotを見つけられず、やるとすると、
IsWin10(True)とかIsWin10(False)みたいな呼び出し方になるので、Iconセクションがわかりにくくなるなぁとおもってあまり悩まず今の実装にしちゃいました。

IsWin10とIsNotWin10の関数名は変えずに、IsNotWin10はIsWin10の呼び出し結果を反転させる実装でいかがでしょう？

m-tmatma · 2018-06-17T08:10:21Z

installer/sakura.iss

@@ -73,7 +76,8 @@ Root: HKCU; Subkey: "SOFTWARE\Classes\*\shell\sakuraeditor\command"; ValueType:
 Root: HKCU; Subkey: "SOFTWARE\Classes\Applications\sakura.exe\shell\open\command"; ValueType: string; ValueName: ""; ValueData: """{app}\sakura.exe"" ""%1"""; Tasks: proglist; Flags: uninsdeletekey; Check: CheckPrivilege(false)

 [Icons]
-Name: "{group}\�T�N���G�f�B�^";                                                Filename: "{app}\sakura.exe";                         Components: main; Tasks: startmenu;
+Name: "{group}\�T�N���G�f�B�^";                                                Filename: "{app}\sakura.exe";                         Components: main; Check: isNotWin10; Tasks: startmenu;
+Name: "{userstartmenu}\�T�N���G�f�B�^";                                        Filename: "{app}\sakura.exe";                         Components: main; Check: isWin10;Tasks: startmenu;


isWin10 で直接判定するのではなく、トップレベルに sakura.exe を
配置するかどうかという関数で判定するようにして、

その関数のなかで、isWin10を呼ぶようにすれば
メンテしやすくなると思います。

１分悩んで理解しました。
isInGroup(True/False)みたいな関数にして、そのなかで、isWin10の判定をするってことですね？
確かにそれがいいかも。

m-tmatma · 2018-06-17T09:01:40Z

installer/sakura.iss

-     (Version.Minor = 0) and
-     (Version.ProductType = VER_NT_WORKSTATION) then
+  if (Version.Major = 10)
+{     (Version.Minor = 0) and


関数名は変わっていて判定処理は変わってませんが、対応途中?

あ、えっと、Majorバージョンのみチェックになってます。
{}でかこむと、ブロックコメント。

メジャーバージョンのみチェックでかつ、>=がいいってことすかね？

はい。

また不要なコードはコメントアウトせずに
削除してしまっていいと思います。

{}でかこむと、ブロックコメント。
それは知りませんでしたが、

コメントはセミコロンのほうが読みやすいと思います。
https://stackoverflow.com/questions/23708355/how-to-comment-a-block-of-statements-in-inno-setup
http://www.jrsoftware.org/ishelp/index.php?topic=scriptformatoverview

私も普段は行コメント使うんですが（IDEで一括行コメントとかよくつかう）、
今回復活させやすいようにとおもってブロックコメントにしちゃいましたが、
いっそ消しちゃいます。

m-tmatma · 2018-06-17T13:05:48Z

installer/sakura.iss

+function IsWin10OrLater : Boolean;
+var
+  Version: TWindowsVersion;
+  S: String;


この変数使ってますか？

Sですよね。
つかってないっす^^
完全に盲点にはいってました。

m-tmatma · 2018-06-17T13:11:33Z

installer/sakura.iss

+begin
+  if ( TopMenu = True ) then
+  begin
+    if ( IsWin10OrLater = True ) then


pascal の文法知らないのですが
引数なしの関数ってカッコ付けないもんですか？

pascal はコピペしたことしかないんですが、if ( IsWin10OrLater = True ) then が気になってます。

sakura/installer/sakura.iss

Line 137 in 11cb421

if IsAdminLoggedOn then

if IsWin10OrLater then という書き方ができそうな気がします。

また週末までさわれなさそうなのですができるところを通勤時間中に返信、
@berryzplus さん
確かにその書き方できますが、普段から何個も言語使ってると、そのif文の期待してる変数がbool変数でその真偽を期待してたのかコーディングミスなのかどうかわかんなくなってくるので、あえて明示的に書くようにしてる「俺様ルール」です。
俺様ルールなので修正いたします。

＠m-tmatma さん
Pascal(というかDelphi Object Pascal)は変数も関数もおんなじ様に使おうって文化だったような。
Pascalの文法を語れるほど精通してるわけではありませんが。
あれですよねCだと、関数名()みたいにしないと文法エラーで気持ちわるいんすよね。
Pascalだと今ためせてませんが慣例的に関数の宣言で引数無しを()とかかくとエラーになってたような。
そして呼び出す側もそれに合わせてたようなおぼろげながらの記憶が。

ただご懸念の通りその名前が定数なのか変数なのか関数なのか見ただけだと分かんなくなるので、自衛のために頭にc定数、lローカル変数、f関数なんてネーミングでわかるようにしてた記憶が。
だいぶDelphi触ってなかったのでissの他の部分に合わせたのもありますが、ここはC使いばかりなのであまり違和感ないここルールで決めてもいいかもですね、Pascal登場するのはissだけだとおもうので。
他のDelphiサンプルもうちょっといい例さがしてみます。
DelphiはDelphiで書かれてるのでディープなサンプル探しやすいっす。
通勤時間もう少しあるので。

とりあえず本家解説、
http://docwiki.embarcadero.com/RADStudio/Berlin/ja/手続きと関数の呼び出し（Delphi）

次の呼び出しは同じです：

DoSomething();
DoSomething;

でも引数無しじゃなくてデフォルトで呼び出す意味らしい、知らんかった（笑）

でも()付きで引数無し関数を呼び出してるところ見かけた記憶がおそらくないので、もしかしたらC臭くなるのを嫌ってるのかもしれませんね。

ここルールで決めてもいいかもです。
もうちょっと他のサンプル探してみます。

趣味は普段一人でしかコーディングしてないのでこのアウェー感たまりません（笑）（いい意味で）

http://u670.com/delphi_tips/tips0044.html

この例が全てではないとおもいますが、
引数無い場合()は省略がルールのようです。

berryzplus · 2018-06-17T13:52:29Z

installer/sakura.iss

+    end else begin
+      Result := False;
+    end
+  end else begin


しょうもない話なんですが、インデントが少し気になりました。

sakura/installer/sakura.iss

Lines 118 to 125 in 11cb421

if admin then

begin

Result := IsAdminLoggedOn;

end

else

begin

Result := not IsAdminLoggedOn;

end;

たぶん、ifとelse、beginとendで揃える慣習みたいなのがあるんではないかと思います。
pythonだとビルドエラーになってくれるから分かりやすいんですけどね。

こちらはですね、サンプル集とかみてもらうと、
http://mrxray.on.coocan.jp/Delphi/plSamples/015_PrintDlgAPI.htm
begin endってCでいうところの{}ぐらいな感覚だとおもってます。
流行りのフォーマッタで整形してもbegin endは字下げされなかったかなと。
ただ人によるかもしれません。
私字下げをTab4にしてたのであまりインデント深くなるとしんどくなったり（笑）

delphiでオートフォーマットしてくれない感じなら対応不要でいい気がします。
これこそ誰かの「俺ルール」だと思うので、やりやすいようにしたらいいです。

ifとelse、beginとendで揃える書き方ってPLSQL系だったかも知れない。

はい、ではインデントはこのままで。

KENCHjp · 2018-06-19T00:43:22Z

ではいったんフィックスで、

この PR マージの前に、 Appveyor でビルドできる状態にしておきたいです。

これ待ちって感じでしょうか。
（そもそもマージの方法がわかってない・・・）

m-tmatma · 2018-06-19T12:38:56Z

この PR マージの前に、 Appveyor でビルドできる状態にしておきたいです。

#144 で PR 投げました。

KENCHjp · 2018-06-23T07:12:25Z

こいつマージしていいのかしら？？？

kobake · 2018-06-23T07:28:09Z

どなたかがこの PR を LGTM とみなして approve すればマージできます。
僕はまだ内容把握してなくて（あと時間もなくて）今はレビューできないです、すみません。

この PR マージの前に、 Appveyor でビルドできる状態にしておきたいです。

これは既に解決済みという認識。

この PR をレビューできそうなのは @berryzplus さん、 @m-tmatma さん、あたりですかね？
余裕のあるときにレビュー＆問題なければ approve && merge お願いします。

m-tmatma · 2018-06-23T07:52:55Z

動作確認してから、対応します

m-tmatma · 2018-06-23T07:55:13Z

できれば、master に対して rebase してほしいです

kobake · 2018-06-23T07:58:17Z

動作確認してから、対応します

ありがとうございます 🙏

できれば、master に対して rebase してほしいです

これ必要です？特にコンフリクト発生していませんが。

kobake · 2018-06-23T08:00:10Z

@KENCHjp
対応内容とは無関係なツッコミですが、ブランチ名に WIP って付けるのは普通はしないですｗ（この話、別の人にもした記憶があります）

WIP を付けることがあるのはプルリクエストのタイトルに対してだけですね。

ブランチ名については大した問題ではないので今回はこのままで良いですが、今後意識していただければと。

m-tmatma · 2018-06-23T08:16:24Z

これ必要です？特にコンフリクト発生していませんが。

rebase していただけたら、最新の master をベースに appveyor でビルドが走ると思ったからです。
動作確認するために #144 が必要だからです。

m-tmatma · 2018-06-23T08:40:24Z

rebase していただけたら、最新の master をベースに appveyor でビルドが走ると思ったからです。
動作確認するために #144 が必要だからです。

ローカルで master をマージして動作確認しました。

Windows 10 Pro でインストールに成功すること
Windows 10 Pro でサクラエディタの EXE が上記のようにトップレベルに表示されること
インストーラのファイル名が sakura_install2-3-2-0.exe になること
アンインストールできること(ログオフ後に C:\Program Files (x86)\sakura が消えること)
アンインストール後にインストールできること

を確認しました。

Windows 7 で確認できる方に確認をお願いしたいです。

kobake · 2018-06-23T09:09:29Z

↑ちょっと間違いあったので修正。

　未使用変数削除　Bool変数判定if文修正

KENCHjp · 2018-06-23T09:14:25Z

あれ、なんか、赤いなぁ。。。

C:\USR\WORKSPACE\GIT\GitHub\sakura>git remote add sakura https://github.com/sakura-editor/sakura

C:\USR\WORKSPACE\GIT\GitHub\sakura>git fetch sakura
remote: Counting objects: 93, done.
remote: Compressing objects: 100% (23/23), done.
remote: Total 93 (delta 55), reused 76 (delta 54), pack-reused 16
Unpacking objects: 100% (93/93), done.
From https://github.com/sakura-editor/sakura

[new branch] ansi -> sakura/ansi
[new branch] master -> sakura/master
[new branch] reimport-installer-with-log -> sakura/reimport-installer-with-log
[new branch] x64 -> sakura/x64

C:\USR\WORKSPACE\GIT\GitHub\sakura>git rebase sakura/master
First, rewinding head to replay your work on top of it...
Applying: Windows10メニュー対応
Applying: Windows10と判定したときのsakura.exeのショートカットを相対パス指定だったのをuserstartmenuに修正
Applying: レビュー結果反映
Applying: Windows10判定を10以降に修正。
Applying: Windows10の判定をMajorのみ10以上判定にする。
Applying: 今回修正分不要コード削除
Applying: PRレビュー結果反映

C:\USR\WORKSPACE\GIT\GitHub\sakura>git push origin feature/WIP-installer-Windows10-Menu -f
Counting objects: 121, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (120/120), done.
Writing objects: 100% (121/121), 536.09 KiB | 7.15 MiB/s, done.
Total 121 (delta 79), reused 0 (delta 0)
remote: Resolving deltas: 100% (79/79), completed with 13 local objects.
To https://github.com/KENCHjp/sakura

43d0aed...aad3530 feature/WIP-installer-Windows10-Menu -> feature/WIP-installer-Windows10-Menu (forced update)

C:\USR\WORKSPACE\GIT\GitHub\sakura>

kobake · 2018-06-23T09:18:04Z

赤いです？テキストだけを見る限りは成功してるっぽく見えます
18:12 に新規ビルドも走ってますね

KENCHjp · 2018-06-23T09:20:31Z

ああ、上のReview requiredとか、Merging is blockedが赤いのにびびってます。

kobake · 2018-06-23T09:21:33Z

あ～、rebase することで PR のコミット内容に変化が起きたので、
一度 approve された状態はリセットされて、非 approve 状態に戻る仕様になってます。

なので再度レビュアーの人に approve してもらう必要があるという感じです。

kobake · 2018-06-23T09:24:22Z

ところで GitHub の Tips を共有しておくと、
ソースコードとかコマンドラインとかの文字列を貼り付けるときには

こういうふうに↑バッククォート3個で囲ってあげると、以下のように枠付きで等幅フォントになってそれっぽい見た目になります。

aaaa
bbbb

ちなみにこの書き方は Slack とか Discord とかでも使えます。あと .md ファイル内でも使えます。

m-tmatma

appveyor のバイナリでもう一度確認しました。
スクリーンショットは省略。

KENCHjp · 2018-06-23T09:26:42Z

https://ci.appveyor.com/project/sakuraeditor/sakura/build/1.0.217

こいつからもってきてインストール確認しました。

kobake · 2018-06-23T09:27:52Z

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

kobake · 2018-06-23T09:28:32Z

こういうふうにレビュアーでない人（approve した人でない人）がマージしちゃうことも可能です（という豆知識）

KENCHjp · 2018-06-23T09:29:43Z

うーん、ほぼついていけてない(笑)

やろうとしていることはなんとなくわかってきてはいるような気もするのですが、各々のコマンドの意味がまだ分かってないからだめですねぇ。

https://liginc.co.jp/web/tool/79390

この辺概念頭にいれて個々のコマンド見直しますです。

kobake · 2018-06-23T09:31:10Z

「初心者でもわかる！」← 嘘です。初心者には普通わかりません。でも覚えておいたほうが良いのでゆっくり習得していきましょう 👍

KENCHjp · 2018-06-23T09:33:53Z

ですねー。一応ブランチとか基本的な概念は理解してるつもりなんですが、ちょっとづつ勉強しますです。
とりあえず初コミッターって感じで（いや本体じゃないけど(笑)）

kobake · 2018-06-23T09:36:09Z

コミットログに名前が刻まれていればコミッターです！ 😆

KENCHjp · 2018-06-23T09:41:23Z

マージ前のrebase で、なんで、appveyor が走っちゃうんだろうってのが、腑に落ちてない。。。
私のブランチで走ったのか、masterへの変化で走ったのか。。。(rebaseだからmaster側？)

kobake · 2018-06-23T09:43:04Z

この PR は KENCHjp:feature/WIP-installer-Windows10-Menu ブランチの内容をチェックするためのものなので、KENCHjp:feature/WIP-installer-Windows10-Menu ブランチに変化があればそれを検出して AppVeyor が走る仕組みになってます。

タイミングとしては git push origin feature/WIP-installer-Windows10-Menu -f のときですね。

kobake · 2018-06-23T09:43:41Z

rebase とか merge とかしなくても、単純に「追加コミットを積んでプッシュしなおす」とかでも AppVeyor ビルドは走ったりします。

KENCHjp · 2018-06-23T09:47:24Z

ああ、そういうことか、masterのリポジトリの変化を監視してるんじゃないってことなんすね。
ちょっと前進。

…er-Windows10-Menu Windows10メニュー対応

KENCHjp mentioned this pull request Jun 17, 2018

インストーラーパッケージ作成運用たたき台 #121

Closed

8 tasks

m-tmatma mentioned this pull request Jun 17, 2018

Installer のx64 対応を行う #131

Closed

m-tmatma added this to the next release milestone Jun 17, 2018

m-tmatma added the installer installer 関連 label Jun 17, 2018

m-tmatma reviewed Jun 17, 2018

View reviewed changes

berryzplus mentioned this pull request Jun 17, 2018

sakura.iss を UTF-8 にする #132

Closed

m-tmatma reviewed Jun 17, 2018

View reviewed changes

berryzplus reviewed Jun 17, 2018

View reviewed changes

m-tmatma mentioned this pull request Jun 19, 2018

appveyor でのインストーラビルド対応 #144

Merged

m-tmatma assigned KENCHjp Jun 20, 2018

m-tmatma mentioned this pull request Jun 21, 2018

バージョン番号の一番末尾の番号にビルド番号を入れる #153

Closed

m-tmatma mentioned this pull request Jun 23, 2018

[x64対応] master の修正を x64 ブランチにマージする #161

Merged

KENCHjp added 7 commits June 23, 2018 18:11

Windows10メニュー対応

3fe33ad

Windows10と判定したときのsakura.exeのショートカットを相対パス指定だったのをuserstartmenuに修正

802cf2b

レビュー結果反映

db96aaa

Windows10判定を10以降に修正。

f1a4e3c

Windows10の判定をMajorのみ10以上判定にする。

fafcfaf

今回修正分不要コード削除

89784ab

PRレビュー結果反映

aad3530

　未使用変数削除　Bool変数判定if文修正

KENCHjp dismissed m-tmatma’s stale review via aad3530 June 23, 2018 09:11

m-tmatma approved these changes Jun 23, 2018

View reviewed changes

kobake merged commit ed6f99c into sakura-editor:master Jun 23, 2018

KENCHjp deleted the feature/WIP-installer-Windows10-Menu branch June 23, 2018 09:30

ds14050 added the installer installer 関連 label Sep 18, 2018

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

Merge pull request sakura-editor#130 from KENCHjp/feature/WIP-install…

402b862

…er-Windows10-Menu Windows10メニュー対応

	if admin then
	begin
	Result := IsAdminLoggedOn;
	end
	else
	begin
	Result := not IsAdminLoggedOn;
	end;

Windows10メニュー対応 #130

Windows10メニュー対応 #130

Conversation

KENCHjp commented Jun 17, 2018 • edited Loading

m-tmatma commented Jun 17, 2018

KENCHjp commented Jun 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KENCHjp Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KENCHjp commented Jun 19, 2018

m-tmatma commented Jun 19, 2018

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

m-tmatma commented Jun 23, 2018

m-tmatma commented Jun 23, 2018

kobake commented Jun 23, 2018

kobake commented Jun 23, 2018

m-tmatma commented Jun 23, 2018

m-tmatma commented Jun 23, 2018

kobake commented Jun 23, 2018

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

kobake commented Jun 23, 2018

m-tmatma left a comment

Choose a reason for hiding this comment

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

kobake commented Jun 23, 2018

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

KENCHjp commented Jun 23, 2018

kobake commented Jun 23, 2018

kobake commented Jun 23, 2018

KENCHjp commented Jun 23, 2018

KENCHjp commented Jun 17, 2018 •

edited

Loading

KENCHjp Jun 17, 2018 •

edited

Loading