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

Added new viewresolver #7

Merged
merged 3 commits into from Nov 23, 2013

Conversation

Projects
None yet
3 participants
@kazuki43zoo
Contributor

kazuki43zoo commented Oct 13, 2013

Hi, I tried to add the new view resolver for the spring-webmvc.

This view resolver is resolve template file & view class by naming rules of view name.

If merge this pull request ,need to add test case & some refactoring.
ex) logic, package, class name, javadoc comment, etc...

Do you think this view resolver.

@buildhive

This comment has been minimized.

buildhive commented Oct 13, 2013

nabedge » mixer2 #178 SUCCESS
This pull request looks good
(what's this?)

@buildhive

This comment has been minimized.

buildhive commented Oct 13, 2013

nabedge » mixer2 #179 SUCCESS
This pull request looks good
(what's this?)

@nabedge

This comment has been minimized.

Owner

nabedge commented Oct 14, 2013

おおっと!知り合いからのプルリク!
サンクスです。超かっちょいいと思います。以下込み入ってるので日本語で^^;)

javadoc(japanese/english)と、テストも書いていただければ、
mixer2-sampleとともにマージしてしまおうと思います。
(最低限の1メソッドだけあるテストケースでもいいです)
パッケージも、org.mixer2.spring.webmvc.* でよいと思います。
古い方のAbstractMixer2XhtmlViewとViewResolverは同時に@deprecatedにしちゃいます。

ちょっとだけ気になったことが1点。
全体的にスレッドセーフになってますよね?
fruitshopサンプルのほうもみましたが、新しいAbstractMixer2XhtmlView(を継承してつくるビュークラスも)
スレッドセーフ性も考慮されているように見えたので大丈夫だとは思いますが、念のため。

実は旧来のAbstractMixer2XhtmlViewは、実装する人によっては非スレッドセーフに作られてしまいそうな
気がしたので、安全なほうに倒すために@scope("prototype") をつけるように
サンプルにもそう書いておいたのです。
#という認識と方法が正しかったのかはさておき。。。

@kazuki43zoo

This comment has been minimized.

Contributor

kazuki43zoo commented Oct 14, 2013

どもです。
そして、日本語で・・。

ライブラリ側のクラスはスレッドセーフな実装になっているはずです。
ただ、ViewResolverで生成されるViewオブジェクトは、view名単位でsingletonなオブジェクトが生成されるので、個別のView実装では、フィールドにthread unsafeなオブジェクトをもたないようにする必要があります。

一応、AbstractCachingViewResolver#setCache(false)にすれば、prototypeなオブジェクトとして生成することもできますが、オススメはできないかな〜と思っています。

テストケース+JavaDoc+リファクタリングも対応して再度pushします。
※たぶん・・・来週とかになる予感です。

ではでは。

@nabedge

This comment has been minimized.

Owner

nabedge commented Oct 14, 2013

了解です。mixer2-sampleのほうはjavadocとかは無くてもいいですよ。
では、いつでもお待ちしてます。
thank you !

nabedge added a commit that referenced this pull request Nov 23, 2013

Merge pull request #7 from kazuki43zoo/added_new_viewresolver
マージしてからちょい手直し開始!

@nabedge nabedge merged commit d08bdca into nabedge:master Nov 23, 2013

nabedge added a commit that referenced this pull request Nov 24, 2013

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