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] UIViewAnimationOptions -> UIViewAnimationCurve #3

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@chuganzy

chuganzy commented Apr 5, 2016

No description provided.

@chuganzy chuganzy changed the title from UIViewAnimationOptions -> UIViewAnimationCurve to [FIX] UIViewAnimationOptions -> UIViewAnimationCurve Apr 5, 2016

@morizotter

This comment has been minimized.

Show comment
Hide comment
@morizotter

morizotter Apr 5, 2016

Owner

Thanks for your pr.
I want to know why do you use UIViewAnimationCurve in this case? Because animateWithDuration:delay:options:animations:completion:'s options is UIViewAnimationOptions so I use UIViewAnimationOptions. And UIViewAnimationCurve is from 2.0, UIViewAnimationOptions is from 4.0. But I don't have other reasons more over these.

I just want to know why thanks:)

Document:
dash

Oh, I found the document:

dash

Owner

morizotter commented Apr 5, 2016

Thanks for your pr.
I want to know why do you use UIViewAnimationCurve in this case? Because animateWithDuration:delay:options:animations:completion:'s options is UIViewAnimationOptions so I use UIViewAnimationOptions. And UIViewAnimationCurve is from 2.0, UIViewAnimationOptions is from 4.0. But I don't have other reasons more over these.

I just want to know why thanks:)

Document:
dash

Oh, I found the document:

dash

@chuganzy

This comment has been minimized.

Show comment
Hide comment
@chuganzy

chuganzy Apr 6, 2016

@morizotter

日本語で失礼します。

screen shot 2016-04-06 at 09 30 24

UIKeyboardAnimationCurveUserInfoKeyにはドキュメントにもあるようにUIViewAnimationCurveの値が入っています。実際にはiOS9では「7」が入ってきています。
UIViewAnimationCurveUIViewAnimationOptionsは別物です。
UIViewAnimationCurveUIViewAnimationOptionsとして使うには、<< 16などの対応が必要で、いづれにせよ現状の実装が間違えていると思います。こちらが分かりやすいと思います。
ビットシフトによる対応は、「多分AppleはanimateWithDuration:delay:options:animations:completion:などの中で>> 16してUIViewAnimationCurveに変換してるだろう」という憶測からのものなので、確実なのは現PRの実装(旧アニメーションのAPIを使う)かなと思っています。

昔それぞれのCurveを比べてみたアプリを作ったのでご参考までに。
https://github.com/chuganzy/EasingDemo

chuganzy commented Apr 6, 2016

@morizotter

日本語で失礼します。

screen shot 2016-04-06 at 09 30 24

UIKeyboardAnimationCurveUserInfoKeyにはドキュメントにもあるようにUIViewAnimationCurveの値が入っています。実際にはiOS9では「7」が入ってきています。
UIViewAnimationCurveUIViewAnimationOptionsは別物です。
UIViewAnimationCurveUIViewAnimationOptionsとして使うには、<< 16などの対応が必要で、いづれにせよ現状の実装が間違えていると思います。こちらが分かりやすいと思います。
ビットシフトによる対応は、「多分AppleはanimateWithDuration:delay:options:animations:completion:などの中で>> 16してUIViewAnimationCurveに変換してるだろう」という憶測からのものなので、確実なのは現PRの実装(旧アニメーションのAPIを使う)かなと思っています。

昔それぞれのCurveを比べてみたアプリを作ったのでご参考までに。
https://github.com/chuganzy/EasingDemo

@morizotter

This comment has been minimized.

Show comment
Hide comment
@morizotter

morizotter Apr 6, 2016

Owner

UIViewAnimationCurveとUIViewAnimationOptionsについてちゃんと考えていませんでした。確かにそうですね!

ただ、アニメーションはblock baseが推奨されているので、変換迷うところです。

As you said, UINotification's curve is UIAnimationCurve. I have to think about converting UIAnimationCurve to UIAnimationOptions.

ex: beginAnimations: context:

Use of this method is discouraged in iOS 4.0 and later. You should use the block-based animation methods to specify your animations instead.

あと、変更が既存ユーザーに影響が出るのでちょっと難しい><

Owner

morizotter commented Apr 6, 2016

UIViewAnimationCurveとUIViewAnimationOptionsについてちゃんと考えていませんでした。確かにそうですね!

ただ、アニメーションはblock baseが推奨されているので、変換迷うところです。

As you said, UINotification's curve is UIAnimationCurve. I have to think about converting UIAnimationCurve to UIAnimationOptions.

ex: beginAnimations: context:

Use of this method is discouraged in iOS 4.0 and later. You should use the block-based animation methods to specify your animations instead.

あと、変更が既存ユーザーに影響が出るのでちょっと難しい><

@chuganzy

This comment has been minimized.

Show comment
Hide comment
@chuganzy

chuganzy Apr 6, 2016

@morizotter
そうですね。実際どちらが良いかは悩ましいですよね…。
必要であれば<< 16での対応にします(READMEも戻します)ので仰って下さいm(_ _)m

chuganzy commented Apr 6, 2016

@morizotter
そうですね。実際どちらが良いかは悩ましいですよね…。
必要であれば<< 16での対応にします(READMEも戻します)ので仰って下さいm(_ _)m

@morizotter

This comment has been minimized.

Show comment
Hide comment
@morizotter

morizotter Apr 6, 2016

Owner

今回は、beginAnimations: contextの変更がライブラリの趣旨(Appleの推奨でかつシンプル)に沿わなかったため、マージしませんでした>< すいません(微変更だったらコメントでのやり取りで修正してもらうなどもありだと思うんですが)。

ただ、頂いたUIAnimationCurveの指摘は確かにその通りで、それに合わせて修正しました。

また、細かい点でも良いのでprなどいただければ!!今回はありがとうございました!

とりあえず修正は、ちょっと既存ユーザーに影響するので、pre-releaseという形で後ほどアップデートかけます。 0.4.0としてリリースしました 😁

Owner

morizotter commented Apr 6, 2016

今回は、beginAnimations: contextの変更がライブラリの趣旨(Appleの推奨でかつシンプル)に沿わなかったため、マージしませんでした>< すいません(微変更だったらコメントでのやり取りで修正してもらうなどもありだと思うんですが)。

ただ、頂いたUIAnimationCurveの指摘は確かにその通りで、それに合わせて修正しました。

また、細かい点でも良いのでprなどいただければ!!今回はありがとうございました!

とりあえず修正は、ちょっと既存ユーザーに影響するので、pre-releaseという形で後ほどアップデートかけます。 0.4.0としてリリースしました 😁

@morizotter

This comment has been minimized.

Show comment
Hide comment
@morizotter

morizotter Apr 6, 2016

Owner

This change is covered by #4 .

Owner

morizotter commented Apr 6, 2016

This change is covered by #4 .

@chuganzy

This comment has been minimized.

Show comment
Hide comment
@chuganzy

chuganzy Apr 6, 2016

@morizotter ホントに細かくて申し訳ありませんが、#4 の修正で修正漏れがありましたのでコメントしました。お時間あるときにご確認下さい🙏

chuganzy commented Apr 6, 2016

@morizotter ホントに細かくて申し訳ありませんが、#4 の修正で修正漏れがありましたのでコメントしました。お時間あるときにご確認下さい🙏

@morizotter

This comment has been minimized.

Show comment
Hide comment
@morizotter

morizotter Apr 6, 2016

Owner

@hoppenichu いえいえ、ありがとうございます。修正しました!ありがとうございました!

Owner

morizotter commented Apr 6, 2016

@hoppenichu いえいえ、ありがとうございます。修正しました!ありがとうございました!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment