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

「カーソル行をウィンドウ上部へ」「カーソル行をウィンドウ下部へ」機能を追加 #1101

Merged
merged 2 commits into from
Nov 30, 2019

Conversation

7-rate
Copy link
Contributor

@7-rate 7-rate commented Nov 29, 2019

PR の目的

「カーソル行をウィンドウの中央へ」ならあるが、上部or下部へ移動する機能がなかったため。

カテゴリ

  • 機能追加

PR の背景

関数名をファイル内検索してヒットした際に、関数全体を見たい。
その時にカーソル行をウィンドウの上部にしたいことが多々ある。
「カーソル行をウィンドウの中央へ」では関数が長い時、全体を俯瞰できない。
また、Page Up/Downだと移動しすぎるし、マクロでも実現できない。

PR のメリット

「PRの背景」で記載した操作などをする人が嬉しくなる。

PR のデメリット (トレードオフとかあれば)

機能が増えて煩わしくなる。

PR の影響範囲

別の機能に影響するものではない。

関連チケット

なし

参考資料

なし

@AppVeyorBot
Copy link

Build sakura 1.0.2403 completed (commit 62e74cd56a by @7-rate)

@usagisita
Copy link
Contributor

ないと不便である、というのには同意しますので、マージ絶対反対とかではない、むしろ賛成派なのですが一応補足します。

マクロでも実現できない

https://sakura-editor.github.io/help/HLP000268.html
ヘルプによればsakura 2.2.0.0からこういう操作もできます。
もっともロジック行レイアウト行とかを理解していて変換できる人に限る、という条件がつくんですけども。まあ一応、ヘルプにもちょっとだけ記述があるからなんとか書けないこともないかなと。SetViewTopの説明に「レイアウト行」って書かれてないのはいただけないですね。

カーソル行をウィンドウ上部へ

var logic_x = parseInt(ExpandParameter('$x'));
var logic_y = parseInt(ExpandParameter('$y'));
var layout_y = LogicToLayoutLineNum(logic_y, logic_x);
SetViewTop(layout_y);

カーソル行をウィンドウ下部へ

var logic_x = parseInt(ExpandParameter('$x'));
var logic_y = parseInt(ExpandParameter('$y'));
var layout_y = LogicToLayoutLineNum(logic_y, logic_x);
SetViewTop(layout_y - GetViewLines());

足し引きすれば、端からのマージンも設定できます。

@berryzplus
Copy link
Contributor

目的は容認、コードの動作的には問題なさげです。
とくに反対意見等つかなければ、そのうちapproveして取り込んでいきたいと思います。

懸念

  1. 「カーソル行をウインドウの上部へ」という機能は、動かない場合がある。(仕様です。
  2. 「カーソル行をウインドウの下部へ」という機能は、動かない場合がある。(仕様です。
  3. 新規作成のはずのコードに、明らかに無関係な人物のクレジットが入っている。当人の許可なく勝手にコメントに名前を入れることは倫理上問題があると思います。

1と2は仕様なので、あとで「バグだ~~~」と騒ぐ人が出る懸念があります。

3は、具体的に指摘しても、「よくわかないから既存コードをコピってきただけです。」と回答されて結局修正はされない事態に陥ることが明白なので、今回はスルーでもよいかも知れないなぁ、と思っています。個人の主義主張としては、よく分らないコードを丸ごとコピーで持ってくる行為は「万死に値する」と考える派なんですが、参加者みんなに「修羅」であることを求めるのはなんか違う気がするので、そこは折れてもよいです。

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

問題無いと思います。

欲を言うと引数で何行ずらすかを設定出来ると良いかもと思いました。

@k-takata
Copy link
Member

CViewCommander::Command_CURLINECENTER() をコピってきて1行だけ変えた関数を2つ作ったように見えますが、それならば共通部分を別関数にくくりだして、3つの関数からパラメータだけを変えて呼ぶようにした方がすっきりするのでは、と思いました。

@arigayas
Copy link

メニューの並びは好みの問題ですが、

  • カーソル行をウィンドウ中央へ
  • カーソル行をウィンドウ上部へ
  • カーソル行をウィンドウ下部へ

ではなく

  • カーソル行をウィンドウ上部へ
  • カーソル行をウィンドウ中央へ
  • カーソル行をウィンドウ下部へ

が良いと自分は思います。

@berryzplus
Copy link
Contributor

#1101 (comment)

CViewCommander::Command_CURLINECENTER() をコピってきて1行だけ変えた関数を2つ作ったように見えますが、それならば共通部分を別関数にくくりだして、3つの関数からパラメータだけを変えて呼ぶようにした方がすっきりするのでは、と思いました。

ぼくが書いてた懸念3はこれを言いたかったです。

コーディングの範疇では「動いているコードをそのまま流用する」は正当なアプローチだと思っています。

共通部分を括り出す行為は、括り出した処理に名前を付けることと同義です。処理に名前を付けるためには、そのコードが「何をしているか?」を把握しておく必要があります。

理想的には、そこまでやるべきだと思います。
共通部分を括り出した状態であれば、新規機能が既存機能とどう違うかも明確になりますし。


懸念の1と2についてですが、「あるべき姿」を検討していくと「ある種のコードを追加する必要があるんじゃないか?」という結論に至ると思っています。

  • 「カーソル行をウィンドウ上部へ」は、カーソル行にある程度行がないと動かない
    • サクラエディタのスクロール範囲には暗黙の制約がある。
      • たとえば、最終行がウインドウ上端になるようにスクロールすることはできない。
    • メニュー項目は本来、「現在のコンテキストで使えるかどうか」の属性を持っている。
      • 実装ではメニュー項目としては常に有効、スクロール操作も常に実行する構成になる。
        • 条件を満たさない場合、スクロール操作は空振る(=動作しない)。
      • スクロールできる条件を満たさない場合にメニュー項目を無効にする選択肢がある。
        • 条件を満たさない場合、スクロール操作を行わない構成が可能(なはず)。

approveも出た感じなので、その辺は今後の課題にして、とくに反対意見等なければそのうちマージな感じで行きたいと思っております。

@7-rate
Copy link
Contributor Author

7-rate commented Nov 30, 2019

レビューありがとうございます。

usagisitaさん、マクロでも実現できたんですね。。補足ありがとうございます。
ただやはり、標準機能としてもあったほうがいいと思うのでこのままPR進めさせてもらいます。

CViewCommander::Command_CURLINECENTER() をコピってきて1行だけ変えた関数を2つ作ったように見えますが、それならば共通部分を別関数にくくりだして、3つの関数からパラメータだけを変えて呼ぶようにした方がすっきりするのでは

これは完全に同意ですので対応しました。

懸念の1と2についてですが、「あるべき姿」を検討していくと「ある種のコードを追加する必要があるんじゃないか?」という結論に至ると思っています。

個人的にはそこまでユーザーに違和感を与える動作にはなっていないと思っているため、いますぐ対応しようという気にはならないです。ので、

その辺は今後の課題にして

でいいと思っています。

@AppVeyorBot
Copy link

Build sakura 1.0.2407 completed (commit 238ae66d1b by @7-rate)

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.

対応ありがとうございます。良さげっス:smiley:

  • 「カーソル行をウインドウ下部へ」が表示最下行にならないのは仕様。
  • このPRは初期インストール時のメニューを弄らないので、共通設定でメニューを変更しない限り追加したメニューは出てこないです。(そこは仕様。初期メニューに入れるとまた話がややこしいので先送り 😢
  • デバッグビルド実行時にメニューが青く表示されるのは、ヘルプが用意されていないからです。これはそのうち対処したい感じですが、いまは良いです 😢

@beru
Copy link
Contributor

beru commented Nov 30, 2019

Mergeします。
もし問題が見つかったら別PRで対処します。

@beru beru merged commit e5172cb into sakura-editor:master Nov 30, 2019
@m-tmatma m-tmatma added this to the v2.4.0 milestone Dec 29, 2019
@KENCHjp KENCHjp added the enhancement ■機能追加 label Jan 7, 2020
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…ommand

「カーソル行をウィンドウ上部へ」「カーソル行をウィンドウ下部へ」機能を追加
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.

9 participants