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

コマンドプロンプトを開くためのメニューを実装 #603

Merged
merged 15 commits into from
Nov 16, 2018

Conversation

m-tmatma
Copy link
Member

@m-tmatma m-tmatma commented Nov 8, 2018

#556: コマンドプロンプトを開くためのメニューを実装
(powershell は別 PR で)

#549 の真似をして実装
ヘルプは未実装です。(#599 のマージ後にこの PR に積みます)

@m-tmatma
Copy link
Member Author

m-tmatma commented Nov 8, 2018

ヘルプは未実装です。(#599 のマージ後にこの PR に積みます)

FuncID_To_HelpContextID に ID を足す必要があります。

@berryzplus
Copy link
Contributor

「shellexecute コマンドプロンプト」でググるとこういうのが出てきます。

windows apiの使い方は何語でもほぼ同じです。
http://mrxray.on.coocan.jp/Delphi/plSamples/490_CmdPrompt1.htm

API関数名をググるとhspとかdelphiのブログにたどり着くことはよくあります。
これの例で行くと、「ディレクトリを指定してcmd.exeを起動する方法」が書いてあるわけです。

issueのほうに powershell にも対応して欲しい旨ありました。
これとは別件で同時リリースが良いのかな?と思ってます。

@m-tmatma
Copy link
Member Author

別 PR で対応するのが、いいと思います

@m-tmatma
Copy link
Member Author

パス指定なしで、cmd.exe を指定すると、パスが見つからないと言うエラーになったので、フルパス指定してます。

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.

具体的な箇所にコメントしないと意図が伝わりにくい気がしたのでレビューコメント付けました。

一応、これのステータスはヘルプ分の修正コミット積みまちな認識です。
あと、「エクスプローラーで開く」と同様にメニューアイコンの実装が未な認識です。

メニューアイコンについては「後日」で進めてしまっていいつもりですが 😸

sakura_core/cmd/CViewCommander_File.cpp Outdated Show resolved Hide resolved
sakura_core/cmd/CViewCommander_File.cpp Outdated Show resolved Hide resolved
sakura_core/cmd/CViewCommander_File.cpp Outdated Show resolved Hide resolved
@m-tmatma
Copy link
Member Author

メニューアイコンについては「後日」で進めてしまっていいつもりですが 😸

#607 を作成しました。

@KENCHjp
Copy link
Member

KENCHjp commented Nov 10, 2018

CMD.EXEは、UNCパスはエラーになったかと。
https://futuremix.org/2009/10/unc-support-for-cmd
事前にエラー処理するか(私は事前にエラーチェックしないでそのままエラーになっちゃってもいいかなって思いますが)、pushdするかは好みかなぁ。。。

PowerShellは、
http://ebc-2in2crc.hatenablog.jp/entry/2016/02/02/162648

@m-tmatma m-tmatma force-pushed the feature/open-command-prompt branch 2 times, most recently from 347b646 to 650c461 Compare November 10, 2018 10:19
@m-tmatma m-tmatma changed the title [WIP] コマンドプロンプトを開くためのメニューを実装 コマンドプロンプトを開くためのメニューを実装 Nov 10, 2018
@m-tmatma m-tmatma force-pushed the feature/open-command-prompt branch from 650c461 to a49416a Compare November 10, 2018 12:09
@m-tmatma
Copy link
Member Author

pushdするかは好みかなぁ。。。

pushd は開くたびにドライブを割り当てるのでよろしくないと思います。

@k-takata
Copy link
Member

レジストリをいじればUNCを使うことも可能です。

REGEDIT4

[HKEY_CURRENT_USER\Software\Microsoft\Command Processor]
"DisableUNCCheck"=dword:00000001

ただ、ちゃんと動かない内部コマンドもあったような気がしますが。

@m-tmatma
Copy link
Member Author

ドキュメントも追加しました。

@m-tmatma
Copy link
Member Author

CMD.EXEは、UNCパスはエラーになったかと。

UNC パスの場合にエラーにする対応とメニュー項目を無効化する対応を入れました。
(メニュー項目の無効化だけでいいかもしれませんが)

@m-tmatma
Copy link
Member Author

レジストリをいじればUNCを使うことも可能です。

REGEDIT4

[HKEY_CURRENT_USER\Software\Microsoft\Command Processor]
"DisableUNCCheck"=dword:00000001

ただ、ちゃんと動かない内部コマンドもあったような気がしますが。

この PR で対応しようすると収拾つかなくなるので、やるとしても別 PR かと思いました。

@m-tmatma m-tmatma added this to the next release milestone Nov 10, 2018
@m-tmatma m-tmatma added the enhancement ■機能追加 label Nov 10, 2018
@k-takata
Copy link
Member

k-takata commented Nov 10, 2018

この PR で対応しようすると収拾つかなくなるので、やるとしても別 PR かと思いました。

ユーザーの設定によっては可能なので、SakuraとしてはUNCについては特別な対処は不要ではと思いました。(あるいはヘルプにcmdの設定を記載する程度。)

追記:もしcmdの設定に応じてメニューの項目をon/offするということを考えているのであれば、HKCUだけでなくHKLMの設定も見ないといけないです。

@k-takata
Copy link
Member

cmd /k で複雑なクォートをしていますが、cmd /k cd /d C:\Program Files (x86) とやっても動きますし、クォートは不要な気がします。

@berryzplus
Copy link
Contributor

ググったら出てきた↓ 一番下になんか書いてありますね・・・。
http://pasofaq.jp/windows/command/disableunccheck.htm

UNCパスが割り当て済みのネットワークドライブ配下のパスに一致する場合はCDで行けるのかな?
自宅でネットワーク共有を使わなくなって久しいのですぐに確認できないんですけど。

@k-takata
Copy link
Member

ああ、ちゃんと動かない内部コマンドというのはまさに cd のことでした。
ShellExecute でカレントディレクトリを設定するのであれば、UNCが使えるのですが、cd /d だとダメですね。

Copy link
Member

@k-takata k-takata left a comment

Choose a reason for hiding this comment

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

ifの後の{の位置が統一されていないのも少し気になりました。

sakura_core/cmd/CViewCommander_File.cpp Outdated Show resolved Hide resolved
sakura_core/cmd/CViewCommander_File.cpp Outdated Show resolved Hide resolved
}

std::wstring strFolder(GetDocument()->m_cDocFile.GetFilePath());
::PathRemoveFileSpecW(&*strFolder.begin());
Copy link
Member

Choose a reason for hiding this comment

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

C++はよく分かっていないのですが、std::(w)string の中身をこうやって書き換えるのって、許容されているんでしたっけ?

Copy link
Member Author

Choose a reason for hiding this comment

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

PathRemoveFileSpecW をつかないようにするために
#613 を作成しました。

Copy link
Contributor

Choose a reason for hiding this comment

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

C++はよく分かっていないのですが、std::(w)string の中身をこうやって書き換えるのって、許容されているんでしたっけ?

方針次第の認識です。いまいるメンバーなら「許容」でいいと思います。
std::stringの管理外でメモリを流用する行為なので、後始末が必要です。
そのことを理解して実践していけるならとくに問題はないと思います。

Copy link
Contributor

Choose a reason for hiding this comment

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

自分は理解していませんし怖くてできません。

Copy link
Contributor

@ds14050 ds14050 Nov 13, 2018

Choose a reason for hiding this comment

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

Java でもコピーした方がいいということで廃止されましたし、C++ では新しい規格によってそういう実装が禁止された不可能になったというようなことを聞いた気がしますが、std::string を Copy on Write で実装する例が過去にはあったようです。配列や std::vector を使わない理由がわかりません。

Copy link
Contributor

Choose a reason for hiding this comment

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

自分は理解していませんし怖くてできません。

なんか東仙隊長のセリフを思い出しました。
ま、それはいいとして、ニュアンスは了解しました。

対策案ですがこんな感じかなぁ。

  • TCHAR xxx[MAX_PATH] して strcpy する
  • std::make_unique<TCHAR[]> して strcpy する
  • 上記どちらかの後 独自関数 SplitPath_FolderAndFile を使う
  • パス分割用のグローバル関数を新設する

個人的に #613 の対策案は違うと思います。

Copy link
Contributor

Choose a reason for hiding this comment

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

文字列取得バッファとしてのstd::string - yohhoyの日記

新規格に限定すると許されてしまうわけですが。

Copy link
Member

Choose a reason for hiding this comment

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

stringに格納したところで、c_str() を使ってC関数に渡すことしかしていないですから、

  • TCHAR xxx[MAX_PATH] して strcpy する

で十分なのではと思うわけですが、C++11以降ならば規格上問題ないということであれば、まあいいのかもしれません。(リンク先のように&strFolder[0]と書いた方がすっきりしてると思いますが。)

Copy link
Member Author

Choose a reason for hiding this comment

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

PathRemoveFileSpecW をつかないようにするために
#613 を作成しました。

#613 (comment)
に基づき 008e210 で修正しました。

beru
beru previously approved these changes Nov 16, 2018
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.

動作確認してみたところ問題無く動いていると思います。

共通設定のメインメニューのプロパティシートでは種別の「ファイル操作系」のリストの下の方に「ファイルの場所を開く」と「コマンドプロンプトを開く」の項目がありますが、共通設定のツールバーのプロパティシートには無いのが気になりましたがそもそもアイコンがまだ無いからですねきっと…。

@berryzplus
Copy link
Contributor

共通設定のツールバーのプロパティシートには無いのが気になりましたがそもそもアイコンがまだ無いからですねきっと…。

はい、そうです。

共通設定 ⇒ ツールバータブ のリストに表示させるコードは書いていません。
アイコンを登録する(=tools.bmpを更新する)ときに一緒にやるつもりです。

@m-tmatma
Copy link
Member Author

明日マージするので、反対の人はそれまでにお願いします。

@ds14050
Copy link
Contributor

ds14050 commented Nov 16, 2018

「コマンド ウィンドウをここで開く(&W)」というのは毎日使う機能です。主に svn や git のために。

今サクラエディタで Ctrl+F5 を押すと入力欄には「cmd」とだけ入っていました。出力を取り込まない設定で実行するとファイルの場所でコマンドプロンプトが開くわけですね。反対があるわけがありません。

@berryzplus
Copy link
Contributor

「コマンド ウィンドウをここで開く(&W)」

突っ込んだほうがいいかな?

rMenu.m_nCustMenuItemFuncArr[CUSTMENU_INDEX_FOR_TABWND][n] = F_OPEN_COMMAND_PROMPT;
Menu.m_nCustMenuItemKeyArr[CUSTMENU_INDEX_FOR_TABWND][n] = '\0';

1438行目。ニーモニックが指定されてないです。

//Nov. 9, 2000 JEPRO 「やり直し」とバッティングしていたアクセスキーを変更(R→F)

こういうコメントがありますが、現在のサクラエディタはニーモニックがかぶっても大丈夫なようになっているので、安心してテキトーに決めて大丈夫です。

どうでもいいけど、(&W)なんですね。
「コマンドウインドウをここで開くw」

berryzplus
berryzplus previously approved these changes Nov 16, 2018
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.

既にLGTM出てますが、重ねてapprove出します。

思ったこと何点か書きましたけど、気になったら誰かが対応する、で進めてよりと思っています。

@m-tmatma m-tmatma dismissed stale reviews from berryzplus and beru via 4729cbd November 16, 2018 12:29
@m-tmatma
Copy link
Member Author

既にLGTM出てますが、重ねてapprove出します。

approve いただきましたが、指摘に対応しました。

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.

メニューの項目にアクセスキーが付いて効く事を確認しました。

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.

6 participants