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 SetProgressViewOffset on SwipeRefreshLayout #19767

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Fix SetProgressViewOffset on SwipeRefreshLayout #19767

merged 4 commits into from
Jan 11, 2024

Conversation

PureWeen
Copy link
Member

@PureWeen PureWeen commented Jan 8, 2024

Description of Change

Alternate PR for
#17647

It looks like there are a few things going on here

  1. it looks like there's a bug with the SwipeRefreshLayout. It's kind of odd because they fixed it two years ago, but it's not fixed in the latest androidx packages. If you look at the main branch they've added an else into the nestedscrollview code https://android-review.googlesource.com/c/platform/frameworks/support/+/1734993 which triggers a reset of the indicator so it hides when you scroll up. I tried a few paths to mimic what they are doing but I'm not sure a reliable way to do this. Also, I don't really like the way this looks. The indicator just kind of vanishes. The scaling generally seems like a nicer way to go.

  2. I'm not sure why exactly, but, for whatever reason our RefreshView isn't clipping to bounds. If you use the SwipeRefreshLayout in a platform app you'll see the indicator goes outside the bounds of the refreshview so it hides itself out of view. My guess is that this is related to our layouts vs using androids in the box layouts.

  3. Calling any of the SetProgress* APIs works around the original issue because calling any of those APIs sets the mUsingCustomStart private variable on the SwipeRefreshLayout which causes this path, so the visibility of the spinner will get set to Gone once you've scrolled all the way back to zero.

image

The size of the ToolBar is kind of irrelevant here. Like if you just call this
SetProgressViewOffset(false, ProgressViewStartOffset, ProgressViewEndOffset);

It works "fine"

if you call this
SetProgressViewOffset(false, 0, ProgressViewEndOffset);

It also works fine and then the refresh circle doesn't go above the RefreshView.

I'm not quite sure where y'all got 50 from, and what that's trying to accomplish?

If we want to somewhat replicate the values inside the SwipeRefreshLayout then I think we'd call it like this

SetProgressViewOffset(true, ProgressViewStartOffset, ProgressViewEndOffset - Math.Abs(ProgressViewStartOffset));

Which I just pulled from here

image

That causes the indicator when refreshing to land in the same spot. I like setting the scale to true. I think the non scaling version is odd, especially if there's a toolbar.

Also, I would call SetProgressViewOffset inside the ctor of SwipeRefreshLayout, that way if developers want to call SetProgressViewOffset themselves they don't have to figure out when we call it.

Issues Fixed

Fixes #17610

@PureWeen PureWeen marked this pull request as ready for review January 9, 2024 17:20
@PureWeen PureWeen requested a review from a team as a code owner January 9, 2024 17:20
// https://github.com/dotnet/maui/pull/17647#discussion_r1433358418
// https://issuetracker.google.com/issues/110463864
// It looks like this issue is fixed on the main branch of Android but it hasn't made its way into the packages yet
SetProgressViewOffset(true, ProgressViewStartOffset, ProgressViewEndOffset - Math.Abs(ProgressViewStartOffset));
Copy link
Contributor

Choose a reason for hiding this comment

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

If this was fixed two years ago, but later appears again in the latest AndroidX packages, should we detect the versions with problems and add conditional code only for certain API Levels?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is, is that I have no idea what version of AndroidX this will actually be released on.

It's fixed on the main branch of their code but I don't really know how to track what version of AndoridX it will show up in.

@jsuarezruiz
Copy link
Contributor

Nice description!. Very useful to understand what is happening.

@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jan 10, 2024
@PureWeen PureWeen requested a review from jsuarezruiz January 10, 2024 18:04
hartez
hartez previously approved these changes Jan 10, 2024
@PureWeen PureWeen enabled auto-merge (squash) January 11, 2024 20:56
@PureWeen PureWeen merged commit 3980e66 into main Jan 11, 2024
47 checks passed
@PureWeen PureWeen deleted the fix-17610-alt branch January 11, 2024 21:21
rmarinho added a commit that referenced this pull request Jan 15, 2024
* Fix issue 18110

* Fix SetProgressViewOffset on SwipeRefreshLayout (#19767)

* Fix SetProgressViewOffset on SwipeRefreshLayout

* - update refresh icon to a better color to trigger screen shot comparison

* - remove extra code

* - add better exception

* Fix incorrect defaults when adding non-BindableObject as AbsoluteLayout child (#19839)

* [Windows] Show the Raw HTML for Html Label in case of error (#17470)

* Show the Raw HTML in case of error

* Try to add UITest for Broken HTML

* added optional ReceiveSms permission (#19659)

* iOS Keyboard Scrolling Improvements and UITests (not from fork) (#17670)

* Squash all the iOS Keyboard Improvements

* delay was enough manually, but not for tests

* remove debugging

* Add logic for Disconnect to be more useful

* fix spacing from github merge conflict commit

* accidently removed a var declaration

* Improve UITests and add double scroll for LargeTitles

* Add UITest for the grid star

* Style changes

* more style

* add fixtureTeardown

* only check for LargeTitles in portrait mode

* Add changes for LargeTitles on iPad

* remove the popover code until we get tests

* [ci] Bump XCode to 15.2.0 (#19836)

* [ci] Bump XCode to 15.1.0

* Go to Xcode 15.2

* New xcode 15.1 doesn't support 14.5 simulators

* Add missing AbsoluteLayout section to layout architecture doc (#19841)

* [ci] Don't install xharness globally (#19863)

* dont install xharness globally

* Fix restore

* Update README.md (#19797)

* [xamlg] improve error for Xamarin.Forms namespace (#19683)

* [xamlg] improve error for Xamarin.Forms namespace

Fixes: #18637

Using the old namespace `http://xamarin.com/schemas/2014/forms` was
causing a NRE:

    System.NullReferenceException: Object reference not set to an instance of an object.
    at Microsoft.Maui.Controls.Xaml.XmlTypeXamlExtensions.GetTypeReference[T](XmlType xmlType, IEnumerable`1 xmlnsDefinitions, String defaultAssemblyName, Func`2 refFromTypeInfo)
    at Microsoft.Maui.Controls.SourceGen.CodeBehindGenerator.GetTypeNameFromCustomNamespace(XmlType xmlType, Compilation compilation, AssemblyCaches caches)
    at Microsoft.Maui.Controls.SourceGen.CodeBehindGenerator.GetTypeName(XmlType xmlType, Compilation compilation, AssemblyCaches caches)
    at Microsoft.Maui.Controls.SourceGen.CodeBehindGenerator.TryParseXaml(SourceText text, String uid, Compilation compilation, AssemblyCaches caches, CancellationToken cancellationToken, String targetFramework, String& accessModifier, String& rootType, String& rootClrNamespace, Boolean& generateDefaultCtor, Boolean& addXamlCompilationAttribute, Boolean& hideFromIntellisense, Boolean& xamlResourceIdOnly, String& baseType, IEnumerable`1& namedFields, Exception& exception)
    at Microsoft.Maui.Controls.SourceGen.CodeBehindGenerator.GenerateXamlCodeBehind(ProjectItem projItem, Compilation compilation, SourceProductionContext context, AssemblyCaches caches)
    at Microsoft.Maui.Controls.SourceGen.CodeBehindGenerator.<>c.<Initialize>b__1_1(SourceProductionContext sourceProductionContext, ValueTuple`3 provider)
    at Microsoft.CodeAnalysis.UserFunctionExtensions.<>c__DisplayClass3_0`2.<WrapUserAction>b__0(TInput1 input1, TInput2 input2, CancellationToken token)

In f939049, I inadvertently introduced this NRE, but even after fixing
it, I would instead get:

    FormsNamespace.xaml.sg.cs(15,48): error CS1001: Identifier expected

Due to the namespace, type, and base type all being `null` in this case,
the generated C# is invalid.

Instead of generating C# *at all*, let's emit an error message that says
to use the correct namespace.

I also provided a `Location` for the error message, so we will now know
what file the error originated from.

I attempted to write some test here, but adding an invalid file like
this causes the XAML unit tests project to fail to build. It feels like
we actually need to create a unit test project for the source generator
-- as there isn't one now.

* Add `[Obsolete]` to `FormsUri`

---------

Co-authored-by: Mike Parker <mikeparker104>
Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
Co-authored-by: E.Z. Hart <hartez@users.noreply.github.com>
Co-authored-by: Tim Miller <drasticactions@users.noreply.github.com>
Co-authored-by: Mike Parker <mikeparker104@users.noreply.github.com>
Co-authored-by: Georgy <tarasverq@yandex.ru>
Co-authored-by: TJ Lambert <50846373+tj-devel709@users.noreply.github.com>
Co-authored-by: James Montemagno <james.montemagno@gmail.com>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2024
@Eilon Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[regression/8.0.0-rc.1.9171] RefreshView indicator hidden behind Navigation bar
6 participants