環境対応 #954
closedUNIXぽくないファイルパス指定の OSでPlugin::Photo::Photo::[]が必ずnilを返す
Description
3.5リリースおめでとうございます。
アルファ版参加できず、今更の報告をご容赦ください。
core/plugin/photo/model/photo.rbの24行目にある、uriがローカルファイルかどうかの条件が「先頭が"/"である」ことになっています。
しかしながら、超漢字やMS-DOSなどUNIXっぽくないOSのパス(c:¥や¥¥hoge¥の様な)はこの条件を通り抜けるため、nilが返ってきています。
今後のmikutter on 超漢字やmikutter for DOSを勘案して、ユニバーサルな判定方法に出来ればと考えています。
例えば、下記の様な条件を考えています。
・"http"から始まらないuriは、一律ローカルファイルとして扱う。
・File.exist?(uri)が真のものをローカルファイルとして扱う。
ご検討をお願いします。
Files
Related issues
Updated by toshi_a 初音 almost 8 years ago
なるほどー超漢字やMS-DOSで困りますねーこれは大変だ
確かに判定基準が微妙だなと思っていたので何か考えたいですね。
httpから始まることを条件にするのは一見完璧なんですが、現状相対パスでも実は動いてしまうので、影響がないか確認する必要があります。あとはanonymouse ftpとか行けたと思うんですが、これは多分使ってないと思うのでいいかな…
FileTestもうまく動くとは思うんですが、存在しないファイルを指したローカルパスの時に判定を誤るのと、このためにファイルの存在確認するのなんか変な感じがします。
明日なんか実装してみますね
Updated by toshi_a 初音 about 7 years ago
- Related to バグ #1071: Windows環境にPhotoが対応していなかった問題の修正 added
Updated by toshi_a 初音 about 7 years ago
- Assignee changed from toshi_a 初音 to Satoshi Okuno
- Estimated time changed from 0:02 h to 0:02 h
明日は来ませんでした。ヽ('ω')ノ三ヽ('ω')ノもうしわけねぇもうしわけねぇ
#1071 のパッチなんですが、実質このチケットで挙げられている問題の修正だと思ったのですが、まずはこの認識は正しいですか?
その場合、Windows側の問題を私が見るのは、Windowsもっていないためほぼ無理かなと思っているので、Windowsを持ってる人にレビューしてほしいです。
少なくとも本件についてはパッチをもらえたので、もともとの報告者であるmogunoさんにレビューしてほしいですが、最近余裕ありますかね…
Updated by Satoshi Okuno about 7 years ago
・kagura1050 ておさんのパッチについて&所感
頂いたパッチはこのチケットに対するもので間違いないです。
改善点としては、ドライブレターが小文字の場合も対応したいなと言うところです。
Windowsのパスという観点だとUNC(\\localhost\dir)の対応も必要だと思ってます。
また、Windowsでは頭にパス区切りが来た場合、カレントドライブのルートを示す(相対パスの一種)ので、そこも救う必要があるなぁとか思いました。
真面目に考えると結構深い問題です。(こだわりすぎかも)
・今後の対応について相談
チケット切っておいてなんなのですが、本件はWindowsへの対応ということで、原則Windowsはサポートしないと言うmikutter開発のポリシーに反するかと思っています。
マージにあたり、どんなアプローチがお好みでしょうか。
(1)えいやっとmikutterの一部にしてしまう。
(2)Plugin::Photo::Photo::[]の中のメソッド分割をいい感じにしておいて、Windowsの対応はmikutter-windowsの様なサードパーティプラグインがモンキーパッチすることにする。
(3)Plugin::Photo::Photo::[]の中の処理をgemに切り出して、mikutter本体にWindows依存の処理が入らないようにする。
・Plugin::Photo::Photo::[]の仕様について確認
現状、相対パスを渡すとnilになっちゃいますけど、意図した挙動でしょうか。(絶対パスのみサポートみたいな)
Updated by toshi_a 初音 about 7 years ago
チケット切っておいてなんなのですが、本件はWindowsへの対応ということで、原則Windowsはサポートしないと言うmikutter開発のポリシーに反するかと思っています。
Windowsだから対応しないというわけではなくて、個別のOSへの対応はサードパーティプラグインでやるべき(できるようにすべき)と考えています。そのうえで、今のローカルファイルパスの判定は雑すぎると思っているので、何らかの修正を行ってもいいかなと思いました。
(2)Plugin::Photo::Photo::[]の中のメソッド分割をいい感じにしておいて、Windowsの対応はmikutter-windowsの様なサードパーティプラグインがモンキーパッチすることにする。
これいいですね。もっと極端なOSでもなんとかなりそうです。
ローカルパスの判別を行いたいのは将来Photo以外でもありそうなので、このチケットを見てどうしようかなと思っていました。
細かい話をすると、メソッドではフィルタを作り、OS毎のプラグインでカスタマイズさせるのが良いと思います。そういうことですよね。
・Plugin::Photo::Photo::[]の仕様について確認
現状、相対パスを渡すとnilになっちゃいますけど、意図した挙動でしょうか。(絶対パスのみサポートみたいな)
そのとおりです。相対パスを考慮する必要はありません。
ちょっと待って。0.04時間で解決は無理ぃ。
Updated by Satoshi Okuno about 7 years ago
- File 0001-Windows.patch 0001-Windows.patch added
>細かい話をすると、メソッドではフィルタを作り、OS毎のプラグインでカスタマイズさせるのが良いと思います。そういうことですよね。
いや、普通にモンキーパッチで乗っ取る気でした。
-----------------------------------------
試しに作ってみました。
・パスっぽい文字列を受け取って、Retriever::Uriを返す or 該当しなければnilを返すフィルタを作る。
・フィルタの結果がnilの場合は、現状のUNIXっぽいOS用の判定を行う。
・かぐらさんのパッチをちょいアレンジして、上記機構のサンプルプラグインにしてみました。
こんなイメージですかね?
Updated by toshi_a 初音 almost 7 years ago
- File 0001-Retriever-URI-URI-uri_filter-refs-954.patch 0001-Retriever-URI-URI-uri_filter-refs-954.patch added
良いですね。Windowsも分離できていますし、様々なファイルパスを持った環境に対応できそうです。
言葉足らずだったのですが、 #954-6 の意図は、たんにローカルファイルパスの判定方法をプラグインで拡張可能にすべき、ということでした。Windowsのファイルパス文字列への対応をするとかしないということではなくて、それをプラグインで可能にしたいということです。
そういったプラグインが開発可能になってようやく、それをmikutterにバンドルすべきか、またはどのように提供すべきかという議論に移れると思います。
UNIXのローカルファイルパスも、適合しない環境では邪魔でしかないので、これもプラグイン化して取り外せるようにするべきでしょう。具体的には、WindowsではUNIX pathのサポートを取り外すべきなんじゃないでしょうか。
また、 0001-Windows.patch を見て閃いたんですが、photoに限らずすべての場所で利用できるようにするなら、 Retriever::URI()
を拡張したほうが良いと思いました。具体的には
Retriever::URI('https://mikutter.hachune.net/').to_uri # => #<Addressable::URI:0x3fc5782fd398 URI:https://mikutter.hachune.net/>
Retriever::URI('/dev/null').to_uri # => #<Addressable::URI:0x2b080bbd1b0c URI:file:/dev/null>
Retriever::URI('invalid').to_uri # raises #<Retriever::InvalidURIError: The string is not URI.>
のようなことが出来ます。仮実装したものが添付ファイル(0001-Retriever-URI-URI-uri_filter-refs-954.patch)です。
実際には、このパッチでphoto pluginに実装しているuri_filterは、photoにあるべきではないと思います(じゃあどこにしよう)。
Windowsの対応は書いていませんが、多分いけますよね(環境がないので書けない、誰か頼んだ)。
Updated by あひる 家鴨 almost 7 years ago
0001-Retriever-URI-URI-uri_filter-refs-954.patch を適用したところ、起動できるようになりました。
Windows 向けの拡張を用意していないため、投稿アイコンなど一部アイコンが notfound.png に正しくなっています。
Updated by toshi_a 初音 almost 7 years ago
クラッシュしないようになったのはいいですね。 #954-8 ではWindowsむけの対応は入れていませんが、画像がないならないでnot foundになったほうがいいと思います。
しかし、特にこの変更で振る舞いが大きく変わることはないと考えていたのですが、どうしてこのような振る舞いの違いが発生したかちょっと気になりますね。
Windows持ってる人で追いかけられる人がいたら調べてほしいです。
Updated by kagura1050 てお almost 7 years ago
拙いRuby力(+pデバッグ)で見た感じではload_pixbufでifnoneが(少なくとも)一部のパスとあひる焼きの飯テロ画像で出力されてる感じですかね
それ以上のことはちょっとわかんなかったです
Updated by toshi_a 初音 almost 7 years ago
- Assignee changed from Satoshi Okuno to toshi_a 初音
停滞しているので、Windows持っている人は #954-9 で、具体的にどの画像が表示されて、どの画像がerrorになるか教えてもらえますか。
パッチを適用して起動しているmikutterのウィンドウをスクリーンショットしてくれたらエスパーデバッグします。
Updated by あひる 家鴨 almost 7 years ago
- File キャプチャ - コピー.PNG キャプチャ - コピー.PNG added
- File setting.PNG setting.PNG added
- 投稿ボックスのしいたけと投稿ボタンが notfound.png になる
- 抽出ボタンのアイコンを設定しても notfound.png になる
- 設定のスキンのプレビューが loading になる
一応スクショを挙げます
Updated by toshi_a 初音 almost 7 years ago
- Assignee deleted (
toshi_a 初音)
原因はわかりました。結論から言ってこの挙動の違いは問題ないので、次のフェーズに移れます。
このスレッドが長くなっているので整理すると、残りは最短で、以下のような流れになります。
- Windowsを持っている誰かが、パッチ(0001-Retriever-URI-URI-uri_filter-refs-954.patch)が当たったmikutterむけにwindowsのパスを解決できるプラグインを書き、このパッチでプラットフォームの差異を吸収する余地ができたことをここで報告
- unixパスの解決をどのプラグインでさせるかの議論
- 結論に従ってパッチを書き換え、merge
なので一旦私が出来ることは終わったので、この問題に興味のあるWindowsユーザに1.をお願いしたいです。このプラグインはmikutterにmergeするわけではないので、ドライブレターの対応だけなど、プロトタイプレベルで良いです。あひるがアップロードしたスクリーンショットでnotfoundになっている部分が表示されれば良いでしょう。
表示される画像とnotfoundに置き換わる画像の違い¶
以下、簡単な調査レポートです。
notfound画像に置き換わる箇所¶
notfoundに置き換わってるのは、
Gtk::WebIcon.new(Skin.get_path('post.png'), 16, 16)
みたいなイディオムを使っているところで、Skin.get_pathというメソッドがnotfound.pngのパスを返しているのかなと思いました( source:core/skin.rb )。
Gtk::WebIcon.newに渡されるのは String です。
画像が表示される箇所¶
一方、home_timelineプラグインなどでは
set_icon Skin['timeline.png']
など、 Skin.[] でPhoto Modelを得ているんですが、 source:core/plugin/gtk/gtk.rb#L523 では結局以下のようなコードで、Photo Model(
Skin['timeline.png']
の戻り値相当) を渡しています。tab.add(::Gtk::WebIcon.new(i_tab.icon, 24, 24).show)
Gtk::WebIcon.newには Photo Model を渡しています。
Gtk::WebIcon.newがStringを渡された時の振る舞い¶
Retriever::Modelでないものを渡された場合は、photo_filterをとおしてPhoto Modelを得ようとします。ここで失敗すると notfound 画像を表示します。
photo_filterはローカルパス文字列またはURLのどちらでも受け取れるため、Windowsでは誤判定してしまい、チケットで報告された問題が発生するのでしょう。
Skin.[]は、Plugin::Skin::Image.[]のエイリアスで、スキンファイル名を受け取り、スキンディレクトリを補完してローカルファイルを読み込みます。ローカルパス文字列とURLの判定をしません。
クラッシュしなくなった原因¶
Retriever::URIは、以前はURIが妥当なものかを確認しませんでしたが、パッチではuri_filterの結果が不正な場合に Retriever::InvalidURIError 例外を投げるように変更されています。Fileをopenしようとしてエラーになるのではなく、もっと前の段階で例外がraiseされるので、挙動が変わったと思われます。
Updated by あひる 家鴨 almost 7 years ago
- パッチ(0001-Retriever-URI-URI-uri_filter-refs-954.patch)が当たったmikutterむけにwindowsのパスを解決できるプラグイン
作りました。
windows_local_file_path
ただし、これを適用しても現状は動きません。
問題は2つあります。- mikutter内部のファイルパスの正規表現が間違っている
これに関してはすでに解決済みなので、私の環境では起きませんがパッチを当てないとそもそもこのプラグインまでuri文字列が渡されることはありません。 - URI 文字列がメタ文字として処理されてしまっている
>>> uri = "C:\Users\ahiru\.mikutter\plugin\windows_local_file_path" uri.match(%r<^[a-zA-Z]+:>) #<MatchData "C:"> >>> uri = "C:\Users\ahiru\.mikutter\plugin\windows_local_file_path" uri.match(%r<^[a-zA-Z]+:\\>) nil >>> uri = 'C:\Users\ahiru\.mikutter\plugin\windows_local_file_path' uri.match(%r<^[a-zA-Z]+:>) #<MatchData "C:"> >>> uri = 'C:\Users\ahiru\.mikutter\plugin\windows_local_file_path' uri.match(%r<^[a-zA-Z]+:\\>) #<MatchData "C:\\">
バックスラッシュ一つで区切られている場合、ruby がメタ文字として処理してしまいC:Users\ahiru\hoge
がC:Usershiruhoge
と豆腐に変換されてしまい区切り文字はメタ文字とともに亜空間に消えます。
これが解消されればプラグインとしては正常に動くことを確認しています。
今はこれの解消方法をどうしようか悩んでいるので、この辺わかりそうな人はヘルプをお願いします。
- mikutter内部のファイルパスの正規表現が間違っている
- 議論
ここでやってる - パッチを書き換え
0001-Retriever-URI-URI-uri_filter-refs-954.patch では ファイルパスの正規表現に誤りがあったため、手元で修正しています。あとでこのスレッドに追加パッチとしてアップします。
変更内容的には以下の通りです。- if uri.is_a?(String) && uri.match(%r<\A\w+:>) + if uri.is_a?(String) && uri.match(%r<\A\w+://>)
上記変更を二か所に追加することでfilterにuri文字列が来るようになりました。
詳しくはパッチ投げた時に説明します。
Updated by あひる 家鴨 almost 7 years ago
- File 0001-URI-match.patch 0001-URI-match.patch added
パッチ作りました。
0001-Retriever-URI-URI-uri_filter-refs-954.patch 適用後に 0001-URI-match.patch を適用してください。
/core/lib/retriever/uri.rb の scheme メソッドでマッチ条件に //
がなかったため、Windows のパスの C://hoge
の C
をスキーマとして返していた。//
までマッチ条件にいれることで、本メソッドでの判定にはならず to_uri
メソッドから generate_uri
メソッドを通して、最終的に generate_uri_by_string
メソッドが呼ばれるようになった。
これにより、 generate_uri_by_string
内の Plugin.filtering(:uri_filter, @uri_string)
が呼ばれ、プラグインの filter_uri_filter
がちゃんと呼ばれるようになった。
なお、 /core/plugin/photo/photo.rb のgeneric uri用のfilter_uri_filterにも同様の修正を加えたことによって、windows_local_file_path プラグイン側でパスが処理できるようになった。
Updated by toshi_a 初音 almost 7 years ago
パッチ 0001-Retriever-URI-URI-uri_filter-refs-954.patch で進みそうなので topic/954-generic-path-to-uri-converter を作成しパッチを適用しました。
#954-17 のパッチの変更は妥当だと思うので、このブランチにmergeしてpushしてください。
Updated by toshi_a 初音 almost 7 years ago
#954-16 1.b.
URI 文字列がメタ文字として処理されてしまっている
これはRubyの仕様で、コード内に文字列リテラルをダブルクォートを用いて書くと、バックスラッシュ記法が有効になります。
https://docs.ruby-lang.org/ja/latest/doc/spec=2fliteral.html#string
\a
はこれによるとベルです。バックスラッシュそのものをStringに含めるには \\
という記法を用いるため、 C:\Users\ahiru\hoge
を内容として持つ文字列をRubyの文字列リテラルを用いて表現するには "C:\\Users\\ahiru\\hoge"
と記述するか、バックスラッシュ記法を無視するシングルクォートを利用します。
puts "C:\Users\ahiru\hoge"
puts 'C:\Users\ahiru\hoge'
puts "C:\\Users\\ahiru\\hoge"
C:Usershiruhoge
C:\Users\ahiru\hoge
C:\Users\ahiru\hoge
これはRubyの文法であって、IOやGtkウィジェットによる入出力には当然影響しないので、そもそも問題は発生していないんじゃないでしょうか。
Updated by あひる 家鴨 almost 7 years ago
マージしてプッシュしました。
URI 文字列がメタ文字として処理されてしまっている
なるほど
出力がそう見えてただけなんですね。
Windows 用のプラグインの修正もして、読み込みエラーになっていたものは全て解決されたようです。
Updated by toshi_a 初音 almost 7 years ago
Windows 用のプラグインの修正もして、読み込みエラーになっていたものは全て解決されたようです。
となると、最後に残るのは、各プラットフォームに対応するプラグインをどのように提供するか、という話になってきますね。
私の意見では、unixパス向けに uri_filter フィルタをhookする新たなプラグインを作成しバンドルすべきだと思っています。
そのプラグインは file_path のような、プラットフォームの名前(unixなど)を含まないslugにしておいて、display_requirementsのように、ユーザやディストリビュータが必要に応じて同じslugをもつサードパーティプラグインで上書きするのが良いと思います。
この変更によってWindowsやその他プラットフォームで発生している問題があれば教えてください。とくに、この変更によってLinuxでの動作に問題が出てはいけないので、Linuxでの報告も欲しいです。
Updated by あひる 家鴨 almost 7 years ago
unix向けのパスフィルタプラグインというと、現在のphotoに実装されているものを1プラグインとして外に出すという認識でいいですか?
それでいい場合はサクッと作ります。
ついでにWindowsのファイルパスプラグインのスラグを変更して統一します。
Updated by toshi_a 初音 almost 7 years ago
unix向けのパスフィルタプラグインというと、現在のphotoに実装されているものを1プラグインとして外に出すという認識でいいですか?
そのとおりです。俺がやろうと思ってましが、週末はあまり余裕なさそうなのでやってみてもらっていいですか
Updated by あひる 家鴨 almost 7 years ago
プラグインを作るだけ作りました。
UNIX用ファイルパスプラグイン
Windows用ファイルパスプラグイン
あとで動作検証します。
Updated by toshi_a 初音 almost 7 years ago
- Status changed from 新規 to 実装待ち
- Assignee set to toshi_a 初音
良いですね。UNIX向けはそのままmikutterに取り込ませてもらいます。
Windows向けは、 #954-5 の
Windowsのパスという観点だとUNC(\\localhost\dir)の対応も必要だと思ってます。
を満たしていないように見えますが、これは mikutter file_path プラグイン for Windows で対応について議論できるようになったと思うので、そちらのプラグインで対応してもらえたらと思います。
Updated by あひる 家鴨 almost 7 years ago
UNCは知らなかったです。
GitHubのリポジトリもできたことですし、そちらのissueとします。
- topic/954-generic-path-to-uri-converter ブランチに unixファイルパスの実装をはずすパッチを適用
- UNIX, Windows環境で動作しない部分が出てくるのを確認
- UNIX環境にプラグインを入れて動作することを確認
- Windows環境にプラグインを入れて動作することを確認
上記動作確認したらブランチにプッシュしといた方がいいですかね(UNIXパスの記述削除
あとプラグインの.mikutter.ymlの方は対象のmikutterバージョンを厳密に定義しようと思うのですが、3.5.14でいいしょうか。(リリースされるまでは検証のため3.5.13とかにしておきます
Updated by toshi_a 初音 almost 7 years ago
上記動作確認したらブランチにプッシュしといた方がいいですかね(UNIXパスの記述削除
これは俺がやります。Divaとのmerge作業が単純にはできないためです。
あとプラグインの.mikutter.ymlの方は対象のmikutterバージョンを厳密に定義しようと思うのですが、3.5.14でいいしょうか。(リリースされるまでは検証のため3.5.13とかにしておきます
これは実際にリリースできるまではバージョン分かりませんが、specファイルに書くのはロードしてほしくないバージョンを指定するためで、今回のプラグインは古いmikutterでロードしてもエラーになったりしないので、厳密に書く必要はないと思います。
Updated by あひる 家鴨 almost 7 years ago
確認作業完了しました。
問題なく動いているようでした。
- topic/954-generic-path-to-uri-converter ブランチに unixファイルパスの実装をはずすパッチを適用
面倒だったので該当箇所をコメントアウト - UNIX, Windows環境で動作しない部分が出てくるのを確認
Linux環境でも #954-13 と同様の現象を確認できた - UNIX環境にプラグインを入れて動作することを確認
通常どおり画像が読み込めるようになったのを確認 - Windows環境にプラグインを入れて動作することを確認
Windows環境でも念のため上記の確認をしたが、想定通りの動作でプラグインを入れることで動作を確認
Updated by あひる 家鴨 almost 7 years ago
Retriever::URIも意図したスキーマが取得できたので問題ないようです。
- Windows系
Retriever::URI("c:/perl").scheme #=> 'file' Retriever::URI("c:\\perl").scheme #=> 'file' Retriever::URI("/tmp").scheme #=> not URI Retriever::URI("http://example.com/").scheme #=> 'http'
- UNIX系
Retriever::URI("c:/perl").scheme #=> not URI Retriever::URI("c:\\perl").scheme #=> not URI Retriever::URI("/tmp").scheme #=> 'file' Retriever::URI("http://example.com/").scheme #=> 'http'
Updated by toshi_a 初音 almost 7 years ago
- Status changed from 実装待ち to レビュー待ち
- Assignee changed from toshi_a 初音 to あひる 家鴨
19027158 にて、Photo pluginからuri_filterを取り除きました。
hotfix/3.5 にpushしました。以下のような変更が加わっているため、挙動が変わっていないか念の為確認してください。
また、このチケットでもともと扱っている問題に関心がある人も、このままリリースしようと思っているので、今のタイミングで意見をください。俺はこのチケットを閉じたら3.6の実装に集中するので、以降3.5に関する細かいバグ修正は年内はしません。
UNIXのローカルファイルパスは、ahiruの https://github.com/Na0ki/mikutter_unix_file_path をバンドルしました。
URI文字列を渡された場合ですが、Divaとの互換性を考えて、Retriever::URIに実装することにしました。これは次のような理由で:
- もともとURI文字列をURIオブジェクトに変換することを目的としたメソッドの中でuri_filterが使われているので、uri_filterからこの機能を削除できることにメリットがない
- uri_filter フィルタからURI文字列をURIに変換する機能が失われてサードパーティプラグインからこれを使った場合に不便になるが
- Retriever::URIで使うために定義したので、その都合にあうように仕様を決めてもよい
- この機能はRetriever::URI (Retriever::Model)を通して使われるべき
- uri_filter フィルタからURI文字列をURIに変換する機能が失われてサードパーティプラグインからこれを使った場合に不便になるが
- DivaはPluggaloidに依存しないため、
Plugin.filtering
を呼べない
とくに異論はないかと思いこうさせていただきました。
#954-30
Retriever::URIも意図したスキーマが取得できたので問題ないようです。
ありがとうございます。Divaに同じ変更を適用するので、テストの参考にします。
Updated by あひる 家鴨 almost 7 years ago
hotfix/3.5 確認しました。
akkiesoft, kagura1050, shibafu528 も確認してくれました。
このチケット閉じるタイミングで #1071 も閉じておきますか?
Updated by toshi_a 初音 almost 7 years ago
- Status changed from レビュー待ち to 終了
mergeも完了しているので、両方closeします。