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

Fix #509 「Ctrl+左ボタンダウンからの左ボタンドラッグによる単語選択がちょっとおかしい」 #552

Merged

Conversation

ds14050
Copy link
Contributor

@ds14050 ds14050 commented Oct 15, 2018

Fix #509 「Ctrl+左ボタンダウンからの左ボタンドラッグによる単語選択がちょっとおかしい」

  • Fix: Ctrl+左ボタンダウンのあとファイルの頭に向かってドラッグしていくと、キャレットの移動が範囲選択に先行することがないように。
  • Chg: 右方向に伸ばした選択範囲を縮めるタイミングが、拡大するタイミングと対称になるように。
  • キャレットの実質的移動がなかった場合に、キャレットの移動と選択範囲の更新をサボるように。

バグの認定とあるべき仕様に関して、待ちましたが特に反応がないので、そのままリクエストを出します。詳細は Issue #509 を参照してください。

* Fix: Ctrl+左ボタンダウンのあとファイルの頭に向かってドラッグしていくと、キャレットの移動が範囲選択に先行することがないように。
* Chg: 右方向に伸ばした選択範囲を縮めるタイミングが、拡大するタイミングと対称になるように。
* キャレットの実質的移動がなかった場合に、キャレットの移動と選択範囲の更新をサボるように。
berryzplus
berryzplus previously approved these changes Oct 15, 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.

PRありがとうございます。

内容とくに問題ないと思います。

通常通り、気付いた点にコメント付けました 😄

GetSelectionInfo().ChangeSelectAreaByCurrentCursor( GetCaret().GetCaretLayoutPos() );

// 選択範囲の両端のうちキャレットがある側を単語境界に調整する。
assert(PointCompare(sSelect.GetFrom(), sSelect.GetTo()) <= 0); // (sSelect) from <= to
Copy link
Contributor

Choose a reason for hiding this comment

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

質問です。これって「仕様」な認識ですか? (From <= To)

hime_chan_no_ribbon という単語があって、最後のnにキャレットがある状態で Shift + ← 押していくと、
選択範囲は To <= From な状態になる気がしています。
(directionを表すメンバとかあるんでしたっけ?パッと思い出せない)

shift を押しながらの場合特になんですが、ダブルクリック時は単語を選択したうえで、
近い方の単語境界にスナップしてくれたほうが嬉しい気がします。
結果として、選択後にキャレットが点滅する位置が前側になることもある、と。
(軽くテストしてみた感じ、常に後ろ側に点滅位置がきてる気がしたんで、認識あってるか聞いときたかったです。)

現状の仕様では必ず From <= To になります、だったらそれはそれでいいです。
まぁ、そうでなかったら assert しちゃまずいって話になるんですけどね 😄

Copy link
Contributor Author

@ds14050 ds14050 Oct 16, 2018

Choose a reason for hiding this comment

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

from <= to は ChangeSelectAreaByCurrentCursorTEST の仕様です。キャレットの位置が常に to になる(To <= From になることがある)ことを想定したコードを最初は書いていましたが、そうではありませんでした。なので assert を書いた上で、from <= to を前提としたコードにしています。

補足すると ChangeSelectAreaByCurrentCursorTEST はシミュレーションのためのメソッドで、実際の選択範囲を変更しません。

Ctrl+クリックでキャレットがどこに置かれるかは今回の変更に関係がありません。

ダブルクリックでの単語選択は今回の変更に関係がありません。

@@ -1128,7 +1128,8 @@ void CEditView::OnMOUSEMOVE( WPARAM fwKeys, int xPos_, int yPos_ )
// 一度移動したら戻ってきたときも、移動とみなすように設定
m_cMouseDownPos.Set(-INT_MAX, -INT_MAX);

CLayoutPoint ptNewCursor(CLayoutInt(-1), CLayoutInt(-1));
CLayoutPoint const ptOldCursor = GetCaret().GetCaretLayoutPos();
CLayoutPoint ptNewCursor(CLayoutInt(-1), CLayoutInt(-1));
Copy link
Contributor

Choose a reason for hiding this comment

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

const の位置はスルーします。自由にやればいいと思ってるので。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

理由はありますが一貫した理由ではありません。

	const CLayoutPoint
	      CLayoutPoint

という揃え方をすると、桁揃えのためのスペースがインデントとしてのタブと繋がってしまって、結果としてタブとスペースが混合したインデントという、いやーなものが出現してしまうのを嫌いました。

Copy link
Contributor

Choose a reason for hiding this comment

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

下のコメントに同じ。

最近はインデント揃えの努力を放棄する文化もあるみたいです。constと非constの区別は変数名に語らせよう、という考え方です。ぼく自身はそれもいいなと思ってますが、自由にやればいいと考えてます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ruby は変数名からスコープがわかりますし、定数かどうかもわかります。それを強制しない言語であっても、真似してもいい慣習であると思っています。pull #539 でも定数を大文字で始めたりしています。

一見したときの規則性を求めて桁揃えにはこだわる方です。おそらく IDE の自動整形とは相性が悪いでしょう。自分は思い通りにならない機械は捨てますが、機械に合わせてうまくやる人の方が多いことは理解しています。

Copy link
Contributor

Choose a reason for hiding this comment

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

インデント揃えにこだわり出すと、型名と変数名と二項演算子とセミコロンとコメントの位置まで揃えたくなって 1行80文字の宗派と抗争になりそうです…。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

行頭はこだわり関係なく揃えましょう。インデントとはそういう意味です。桁揃えはほどほどにやります。1行80文字派もそろそろ肩身が狭いはずなので、多少足下は見ますが。

sakura_core/view/CEditView_Mouse.cpp Show resolved Hide resolved
@ds14050
Copy link
Contributor Author

ds14050 commented Oct 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です。

色々書いてますけど「問題なさそう」に影響のある内容じゃないと考えてます。

@ds14050
Copy link
Contributor Author

ds14050 commented Oct 16, 2018

sakura リポジトリの History を見るとコミットの粒度が細かすぎて見にくいので Squash and merge を試してみたいと考えていましたが、ボタンが無効になっていました。

pull #242 の経緯を見る限りでは、著者とコミッタが区別できたので禁止する理由はないものと理解しましたが、どうでしょうか。おそらく>@m-tmatma さん

@m-tmatma
Copy link
Member

sakura リポジトリの History を見るとコミットの粒度が細かすぎて見にくいので Squash and merge を試してみたいと考えていましたが、ボタンが無効になっていました。

有効にしておきました。merge コミットでもいいとは思いますが。

@ds14050 ds14050 merged commit 8881970 into sakura-editor:master Oct 16, 2018
@ds14050
Copy link
Contributor Author

ds14050 commented Oct 17, 2018

Squash and merge できました。ありがとうございます。

未だ Rebase and merge が禁止されている理由を考えてみました。これはコミットが予め整えられていることが前提の選択肢であって、現状でも困っていない @m-tmatma さんにとっても、コミットツリーの系統からコミットより粒度の大きな変更の単位を取り出すことができなくなるのが困るからなのでしょう。もちろん自分も困ります。代わっての適切な判断に、重ねて感謝します。

@m-tmatma m-tmatma added this to the next release milestone Oct 21, 2018
@ds14050 ds14050 deleted the FixWordBoundarySelectionWithMouse branch November 27, 2018 08:30
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019
…ra-editor#552)

* Fix: Ctrl+左ボタンダウンのあとファイルの頭に向かってドラッグしていくと、キャレットの移動が範囲選択に先行することがないように。
* Chg: 右方向に伸ばした選択範囲を縮めるタイミングが、拡大するタイミングと対称になるように。
* キャレットの実質的移動がなかった場合に、キャレットの移動と選択範囲の更新をサボるように。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants