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

Change PageView to derive from ListView #14252

Merged
merged 37 commits into from
Nov 17, 2015
Merged

Change PageView to derive from ListView #14252

merged 37 commits into from
Nov 17, 2015

Conversation

neokim
Copy link
Contributor

@neokim neokim commented Oct 28, 2015

PageView was derived from Layout and it implemented the features of scrolling and item arrangement from scratch. But the features are already there in ListView. So remove those duplicated implementations from PageView and make it subclass from ListView.

By this, PageView becomes more simplified and maintainable because it considers only paging implementation. And the user experiences regarding scrolling are processed in one place - ScrollView.

One more, page view now has a page indicator which is represented as a list of small circles.

Neo Kim added 30 commits October 28, 2015 16:56
…lView::Direction` instead of `PageView::Direction`.
…` and `moveChildrenToPosition()`. All move related logic will be integrated into one.
…s.h` in order to use it in other places.
@zilongshanren
Copy link
Member

@neokim But it also means this PR is incomplete. Anyway, if you could open another PR after this one is merged. I think it's ok for me.

}
}

Layout* PageView::createPage()
void PageView::addWidgetToPage(Widget *widget, ssize_t pageIdx, bool forceCreate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a TAB, please change it to 4 spaces. And it happens many places in the header file and the implementation file.

@neokim
Copy link
Contributor Author

neokim commented Oct 29, 2015

@zilongshanren Ok. I thought the paging scroll threshold can be removed from this PR. I might need to mark it as deprecated APIs.

@zilongshanren
Copy link
Member

@neokim
qq20151029-0 2x
In the UIPageViewVerticalTest:7 , the PageView

Indicator's initial position is wrong.

@zilongshanren
Copy link
Member

@neokim
Regards the original PageView API, it seems you have deprecated almost all of them.

Due to the implementation, I think it make sense to do this change.

But the original PageView API's names indent better than the ListView's API.

Like pushBackCsutomItem doesn't make sense compare to addPage when using PageView.

So I suggest we just keep the old API.

Thoughts?

@neokim
Copy link
Contributor Author

neokim commented Oct 29, 2015

@zilongshanren I'll see the issue about wrong initial position of indicators.

Regarding original PageView APIs, the previous APIs accept and return Layout as its page item. But I think that the type of page item doesn't need to be limited to Layout, it can be any Widget. Overloading the APIs to accept Widget can be an option. But the methods returning Layout like getPages(), getPage() cannot be overloaded to return Widget because overloading is not permitted when only return type is different.

I agree with you about the naming, but I just wanted to be consistent.

I will overload some methods accepting Widget like addPage(), but getPages() still needs to be changed.

@zilongshanren
Copy link
Member

@neokim
OK, add more overload methods is a possible solution. I agree with you that the getPages API should be deprecated.
Thanks.

@liamcindy
Copy link
Contributor

@zilongshanren Taking into account the features of the current editor, these changes cloud be merged.

@zilongshanren
Copy link
Member

@neokim
Hi, do you have time now? Since we have released v3.9, I'm ready to merge this PR now.
But as we discussed here, you owe me another PR 😄

@neokim
Copy link
Contributor Author

neokim commented Nov 12, 2015

@zilongshanren I haven't had much time to look into the issues in this PR. I will wrap it up soon like in early next week.

Regarding to adding magnetic scrolling strength (the PR you saying I owe 😃), I think it should be considered as an independent one. So after I resolve the issues you mentioned in this PR and you merge it, there will be no remaining things in here.

@zilongshanren
Copy link
Member

[ci rebuild]

@neokim
Copy link
Contributor Author

neokim commented Nov 15, 2015

@zilongshanren The issues you mentioned are all resolved. Please review PR again.

…o avoid compilation error on windows platform
@neokim
Copy link
Contributor Author

neokim commented Nov 15, 2015

And removed the CC_DLL in front of createSpriteFromBase64() in ccUtils.h to avoid the compilation errors on windows platform.

@zilongshanren
Copy link
Member

[ci rebuild]

zilongshanren added a commit that referenced this pull request Nov 17, 2015
…listview

Change PageView to derive from ListView
@zilongshanren zilongshanren merged commit 5d8cfa8 into cocos2d:v3 Nov 17, 2015
@zilongshanren
Copy link
Member

@neokim
👍 Good work.
I suggest to add some adapt APIs for addPage, removePage and setCustomScrollThreshold.

Look forward to your next PR. 😄

@neokim
Copy link
Contributor Author

neokim commented Nov 17, 2015

@zilongshanren Thanks.

@neokim neokim deleted the change_pageview_to_derive_from_listview branch November 17, 2015 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants