Add PagerControl #3327
Add PagerControl #3327
Conversation
ghost
added
the
dev/PagerControl/PagerControl.cpp
Outdated
| m_lastPageButtonClickRevoker = lastPageButton.Click(winrt::auto_revoke, { this, &PagerControl::LastButtonClicked }); | ||
| } | ||
|
|
||
| if (const auto comboBox = GetTemplateChildT<winrt::ComboBox>(c_comboBoxName, *this)) |
There was a problem hiding this comment.
GetTemplateChildT [](start = 30, length = 17)
With this method, if a new template is applied after the initial template, which removed this named template part, we would keep the pointer to the old template's part which would likely crash. I would use a lambda here to limit the scope of the local variable while assigning the value of get template child to m_combobox event if the return value is null.
There was a problem hiding this comment.
Very good point, updated that section now.
There was a problem hiding this comment.
The method you chose to do this requires an extra call to m_combobox.get(), which isn't free, which is why i suggested a lambda (since it also scope limits. However this does function.
In reply to: 494660076 [](ancestors = 494660076)
There was a problem hiding this comment.
I'm using lamdas now for the combobox and numberbox.
|
@chingucoding forgot to at you for my last comment as well. |
|
No idea why I added that, since it doesn't make a difference, I removed that now. Thank you for the ping and help in investigating these crashes! |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
Looks like tests were not run because helix timed out. |
@chingucoding If we want to have a regression test, how about writing an API test instead? Then we have an explicit test for that scenario instead of opening the TestUI where a crash could happen due to multipe reasons in theory and not just caused by the PagerControl instance with no properties explicitly set (crash reason easier locatable then). |
|
Great idea @Felix-Dev ! Switch to an API test now. |
|
@chingucoding are you able to look at these test failures? it looks like we are maybe missing a wait for idle. |
|
Taking a look now, thanks for the ping! |
|
This is probably an issue with the selection logic used in the interaction test to select a page through the ComboBox. Strangely, not all tests that rely on this logic seem to fail and even more strangely, if a test succeeded, it succeeded in attempt 3. Unfortunately, I wasn't able to repro this on my machine, so the latest commit is just a guess here. |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
@chingucoding can you please merge master into this branch to pick up the pipeline stability change that went in today? (also if you would do the same for your other PR's that would be great) |
|
Updated all my PRs (except the Progressbar one which still has a passed CI build) to have the latest fixes from master. |
|
/azp run |
|
No pipelines are associated with this pull request. |
… into user/chingucoding/pagercontrol
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This PR removes the prototype Pager and adds the new C++ PagerControl. The specification for this control can be found here.
Motivation and Context
How Has This Been Tested?
New tests and a lot of experimenting.
Screenshots (if appropriate):
Light theme, normal styles:
For fun, customization through available API:
High contrast:
Open questions
If you set the FirstPageButton to Visible, but the PreviousPageButton to HiddenOnEdge should the PreviousPageButton still take up space to prevent the control jumping around?
Current implementation of ComboBox infinity mode is to render 100 items, or if the last NumberOfPages was greater than 100, to stay at that. What would the best way for "infinity mode combobox" be?
Is "infinity mode" a good name?