正規表現キーワードのURLで0文字マッチすると無限ループになる問題に対処

[beru]

PR の目的

#1027 の問題を解消するのが目的です。

カテゴリ

• 不具合修正

PR の背景

#1027 のような背景があります。

処理の流れとしては、

CEditView::OnMOUSEMOVE
CEditView::IsCurrentPositionURL

となっています。マウスカーソルをエディタのウィンドウ上にもってこなければ無限ループは発生しません。

PR のメリット

#1027 の問題が解消します。

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

#1027 の問題が解消してしまいます。

PR の影響範囲

正規表現キーワードのURL

関連チケット

#1027, #1020

[AppVeyorBot]

✅ Build sakura 1.0.2215 completed (commit 1470963c74 by @)

[berryzplus]

範囲外なのでコメントで書きます。

問題があるのは、CEditView::IsCurrentPositionURLメソッドです。 （エロい人には分からんのです！
と結論付けるとして、対応すべき箇所はこっちじゃないでしょうか？

sakura/sakura_core/view/CEditView.cpp

Lines 1247 to 1250 in 832c104

	while( i <= ptXY.GetX2() && i < nLineLen ){
	bMatch = ( bUseRegexKeyword
	&& m_cRegexKeyword->RegexIsKeyword( CStringRef(pLine, nLineLen), i, &nUrlLen, &nMatchColor )
	&& nMatchColor == COLORIDX_URL );

&& nMatchColor == COLORIDX_URL の前でも後でも、
&& nUrlLen > 0 を入れてやれば似た感じの意味になります。
たぶん、こっちに入れたほうが対処したい事象を正確に捉えられる気がします。

[ds14050]

どちらを修正するかを決めるには、「0文字のURLにマッチする正規表現キーワード」が何を意味するのか、どういう結果を期待してそういう設定をするのかを考えなければいけません。それにより修正すべき場所が決まります。それを考えずには決まりません。

[ds14050]

書いてから #1020 (comment) を読みました。ユーザーを想定(「設定するヤツが悪い」)したうえでの指摘なら言うことはありません。消えます。

[berryzplus]

どちらを修正するかを決めるには、「0文字のURLにマッチする正規表現キーワード」が何を意味するのか、どういう結果を期待してそういう設定をするのかを考えなければいけません。それにより修正すべき場所が決まります。それを考えずには決まりません。

おひさしぶり 😄

0文字マッチとなる正規表現を指定することで、何を期待していますか？

機能の特性から指定できる意味はないと考えています。

だから、正規表現キーワードの仕様を修正すべく別PR #1030 を投げました。

何か期待したい有益な効果があって、それを残したいというのなら、もちろん検討するつもりです。わかるように説明してもらえるとうれしいです。

[beru]

範囲外なのでコメントで書きます。

問題があるのは、CEditView::IsCurrentPositionURLメソッドです。 （エロい人には分からんのです！
と結論付けるとして、対応すべき箇所はこっちじゃないでしょうか？

sakura/sakura_core/view/CEditView.cpp

Lines 1247 to 1250 in 832c104

	while( i <= ptXY.GetX2() && i < nLineLen ){
	bMatch = ( bUseRegexKeyword
	&& m_cRegexKeyword->RegexIsKeyword( CStringRef(pLine, nLineLen), i, &nUrlLen, &nMatchColor )
	&& nMatchColor == COLORIDX_URL );

&& nMatchColor == COLORIDX_URL の前でも後でも、
&& nUrlLen > 0 を入れてやれば似た感じの意味になります。
たぶん、こっちに入れたほうが対処したい事象を正確に捉えられる気がします。

そこにその判定を入れると nUrlLen が 0 の場合に bMatch が false になると思いますが、そうすると下記の処理が動くことになりそうです。それを走らせるべきか悩みますね。

sakura/sakura_core/view/CEditView.cpp

Lines 1251 to 1255 in 832c104

	if( !bMatch ){
	bMatch = ( bDispUrl
	&& (i == 0 || !IS_KEYWORD_CHAR(pLine[i - 1])) // 2009.05.22 ryoji CColor_Url::BeginColor()と同条件に
	&& IsURL(&pLine[i], (Int)(nLineLen - i), &nUrlLen) ); /* 指定アドレスがURLの先頭ならばTRUEとその長さを返す */
	}

きちんと検討するとなると今の書き方がそもそもロジックとして妥当かどうかに踏み込むことになりますかね？

[berryzplus]

そこにその判定を入れると nUrlLen が 0 の場合に bMatch が false になると思いますが、そうすると下記の処理が動くことになりそうです。それを走らせるべきか悩みますね。

期待通りの動作です。

正規表現キーワードに一致しなかったなら、
通常のチェックをするんじゃないのかな？
と思っています。

指摘前の変更内容だと if( bMatch && nUrlLen > 0 ){ なので、正規表現キーワードでないほうの判定で得たのURL文字列長もみてしまいます。

IsURLのパスに潜在的な問題があったとしても、「良くはない」なと思います。
対処したい問題と違うところを触ってしまっているので。

[beru]

@berryzplus さんが #1030 で対応している

そこにその判定を入れると nUrlLen が 0 の場合に bMatch が false になると思いますが、そうすると下記の処理が動くことになりそうです。それを走らせるべきか悩みますね。

期待通りの動作です。

正規表現キーワードに一致しなかったなら、
通常のチェックをするんじゃないのかな？
と思っています。

指摘前の変更内容だと if( bMatch && nUrlLen > 0 ){ なので、正規表現キーワードでないほうの判定で得たのURL文字列長もみてしまいます。

IsURLのパスに潜在的な問題があったとしても、「良くはない」なと思います。
対処したい問題と違うところを触ってしまっているので。

まぁどちらも nUrlLen が 0 になるくらいなら true を返すべきでは無いんでしょうね。

それではこちらのPRは閉じます。

[ds14050]

0文字マッチとなる正規表現を指定することで、何を期待していますか？

機能の特性から指定できる意味はないと考えています。

今更ですけど書いておきます。@beru さんが #1028 (comment) で指摘したような違いが生じます。ユーザーから見える違いですから、その元になった設定からユーザーの意図を読み取ることができます。

実際にはこれまでは無限ループする無効な設定だったので、想定すべきユーザーは存在していませんでした、過去には。

無限ループが修正されたときには、「0文字のURLにマッチする正規表現キーワード」が、不可視であるが有効であり他の色分けを隠してしまうのか、無効であり設定されていないのと同じであるのか、という動作・意義付けの違いがユーザーの目に見えるようになるので、それをコードより先に考えなければいけないと考えました。

---

追記@2019-09-05 「不可視」は嘘でした。おそらく現在位置より後ろにあるキーワード終了位置を前に求めた結果なのでしょう、長さ０のキーワードのスタイルが行末まで続くみたいでした(ver. 2.3.2.0)。

このことは本コメントの要諦には影響しませんが、長さ０を想定しないことによるバグっぽい挙動です。pull #1030 で解消しました。

[beru]

正規表現キーワードによるURL判定が通常のURL判定より前に行われて優先される実装になっていますが、もしあるべき仕様の検討を放棄して仮に自由度をひたすら上げる事で対処するとしたら、処理順や有効だけどマッチしなかった場合や 0文字マッチの場合に後続のマッチを実行するかに関しても設定で可変にする、とかでしょうか？ユーザーが設定画面を見た時に「細かすぎてわけわからん…」と感じそうですが。。

[ds14050]

ユーザーが設定画面を見た時に「細かすぎてわけわからん…」と感じそうですが。。

でしょうね。

自由度を求める理由がカンマを含む URL を除外することなら、内蔵のURL強調表示をOFFにする代わりとして、カンマで終わる URL にマッチする正規表現キーワードを「テキスト」に色分けするのもひとつの方法です。

[ds14050]

カンマで終わる URL にマッチする正規表現キーワードを「テキスト」に色分けする

そうしたら URL が URL にならない……。

「内蔵のURL強調表示をOFFにして(これ重要)、正規表現キーワードで色を"URL"に指定すれば、いいんじゃないでしょうか。」

「これ重要」を鵜呑みにして そのうえさらに 血迷ったことを書きましたが、重要な理由がわかっていません。内蔵より正規表現キーワードが優先(※さらに最も左から始まるものが優先)されるのですから。

[beru]

「これ重要」を鵜呑みにして血迷ったことを書きましたが、重要な理由がわかっていません。内蔵より正規表現キーワードが優先(※さらに最も左から始まるものが優先)されるのですから。

#1020 (comment)

１つ前のコメントに対する返信なので、正規表現キーワードが優先されるという前提を意識していない状態でのやり取りなんだと思います。

[berryzplus]

正規表現キーワードで0文字マッチをマッチとみなした場合の効果を考えてみました。

マッチ位置の次の1文字を、他の設定をすべて無視して、強制的に強調表示できる

ですかね・・・。

そうか、思い出したっ！

と思ったけど違ったみたいです...orz

改行コードを色分け表示するための裏ワザだったような。
改行コードって、CRLFでもLFでも同じ色になっちゃうから不便よね、を力技で回避してみる掲示板ネタを見たような・・・気のせい。

[usagisita]

(これ重要)←についてですが、変な正規表現を書いたうえで、変なマッチをしており、
それによって、内蔵と正規表現の優先度を見誤った誤認によるものです。混乱させて申し訳ないです。

0文字マッチを1文字マッチに読み替えることによる利点は、
検索の黄色の強調表示で、正規表現パターンをテストした後、ほぼそのままの形で正規表現キーワードに持ち込むことができるというのはあります。