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 the problem that scroll-other-window raised an error #14

Merged
merged 1 commit into from
Mar 4, 2012
Merged

fix the problem that scroll-other-window raised an error #14

merged 1 commit into from
Mar 4, 2012

Conversation

ambi
Copy link

@ambi ambi commented Mar 4, 2012

scroll-other-window and similar functions raised an error when there is only one window.
By this fix, scroll-other-window, scroll-up-other-window, scroll-down-other-window, scroll-up-both-window, and scroll-down-both-window don't raise an error.
I added unit tests for this fix.

なぜこの修正か?
scroll-other-windowなどはC-M-vやC-S-Up, C-S-Downでキーバインドされている。なので、押し間違いとかで「範囲外の値です」というエラーダイアログが出てしまうことがある。これはちょっとうざいし、一般ユーザはびっくりしてしまう。

問題
以前は、ウィンドウが1つだけのときにエラーが出ていた関数群が、エラーを出さなくなっているので、すっごい細かいけど仕様変更にはなってしまいます。なんで、議論の余地のある変更かも……。ご自由に。

scroll-other-window and similar functions raised an error
when there is only one window.
By this fix, scroll-other-window, scroll-up-other-window,
scroll-down-other-window, scroll-up-both-window, and
scroll-down-both-window don't raise an error.
@mumurik
Copy link
Owner

mumurik commented Mar 4, 2012

また取るかどうか悩ましいfixをw
個人的にはこの動作の方がいいとは思うけど、どうしたもんかなぁ。
2chに書いてみるかな。この手の議論を2chだけでしていいのか、って話はあるが。

@mumurik
Copy link
Owner

mumurik commented Mar 4, 2012

暫定的に取り込んどきます。

mumurik added a commit that referenced this pull request Mar 4, 2012
fix the problem that scroll-other-window raised an error
@mumurik mumurik merged commit 95cc7b3 into mumurik:master Mar 4, 2012
@southly
Copy link

southly commented Mar 11, 2012

個人的にこの修正には賛成できません。理由は以下の三点。

  1. scroll-up-both-window と scroll-down-both-window の動作が説明なしに変更されていること
  2. emacs の scroll-other-window がエラーを投げている以上、あわせられる部分はあわせておいたほうがいい
  3. 分かりにくいながらエラーメッセージを出していたのにそれがなくなっていること

エラーダイアログが出るのはやりすぎだというのであればrange-errorではなく、ダイアログの出ないエラーを投げるようにすると良いと思います。
https://github.com/southly/xyzzy/commit/13da3af3cc88bdd71cd213a538a4ae080383cc9d
上のはother-windowを変更していますが、scroll-other-window等でエラーハンドリングしても良いと思います。

@ambi
Copy link
Author

ambi commented Mar 11, 2012

おお、自分もその変更がベターだと思いますよ。

@mumurik
Copy link
Owner

mumurik commented Mar 11, 2012

ambi氏はいますか?個人的にはどうでもいいので、お二人で好きに決めてもらって良いかと。
もし居ないなら私が話し合いますが、意見のある人同士の方が望ましいかと。
結論が出なければ言ってくれれば私が独断で決めます。

自分としては1,3の理由は既存の動作がおかしいと思っているなら気にしなくて良いと思いますが、2はそれなりの理由が無いなら合わせた方が良いとは思ってます。

元の変更をrevertするべき、という結論ならその作業は私がやっても構いません。(PRくれた方が楽で嬉しいけど)
それ以外の作業はPRという形じゃない限りは受け付けない方針でいきたいと思います。

@mumurik
Copy link
Owner

mumurik commented Mar 11, 2012

なるほど、そんじゃあどちらかPRください。

@ambi
Copy link
Author

ambi commented Mar 11, 2012

southlyさんの方がxyzzyはよく理解しておられると思うので、
やってくださればsouthlyさん、どうぞー。

@southly
Copy link

southly commented Mar 11, 2012

とりあえずやっておきましたが、テストも必要ですか?

@mumurik
Copy link
Owner

mumurik commented Mar 11, 2012

元に戻しているのに近いので、現状の削除、という対応で良いと思います>テスト

mumurik added a commit that referenced this pull request Mar 11, 2012
fix issue #14 (fix the problem that scroll-other-window raised an error)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants