Skip to content

Add lazy loading threshold for ImageEx control #3483

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

Merged
21 commits merged into from
Jan 21, 2021

Conversation

h82258652
Copy link
Contributor

@h82258652 h82258652 commented Sep 18, 2020

Fixes

Add lazy loading threshold for ImageEx control and change the lazy loading implement that will be also fixed #3258 .

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature

What is the current behavior?

If enable lazy loading, only the ImageEx entered the viewport, the source will start to load.

What is the new behavior?

Give a threshold for triggering the lazy loading behavior, the default value is 300 px.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

@ghost
Copy link

ghost commented Sep 18, 2020

Thanks h82258652 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost September 18, 2020 04:49
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Sep 18, 2020
@h82258652
Copy link
Contributor Author

@michael-hawker Done, can you review again?

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Couple of questions. Also, does this mean we can close #3255 or are these independent?

@@ -80,7 +82,7 @@ public ImageExBase()
{
LockObj = new object();

EffectiveViewportChanged += ImageExBase_EffectiveViewportChanged;
LayoutUpdated += ImageExBase_LayoutUpdated;
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to do both still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-hawker Don't need, issue #3258 is occurred by register EffectiveViewportChanged while the control host in a Viewbox, it is a framework bug.

@@ -6,6 +6,7 @@
<Setter Property="Background" Value="Transparent" />
<Setter Property="Foreground" Value="{ThemeResource ApplicationForegroundThemeBrush}" />
<Setter Property="IsTabStop" Value="False" />
<Setter Property="LazyLoadingThreshold" Value="300" />
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you picked 300?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-hawker About this I think a lot. I searched google and it tells me Chromium will calculate this by the screen size and network type. And then I searched the most widely used JavaScript image lazy loading library, got this. This lib use 300px as default. I think UWP is mostly used on a PC, not a mobile device. The network should be not bad, 300px should be suitable for most situations.

@michael-hawker michael-hawker added this to the 7.0 milestone Nov 5, 2020
@ghost
Copy link

ghost commented Jan 14, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Caught this one last thing I think. Then we should be good.

@ghost
Copy link

ghost commented Jan 20, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@ghost ghost removed the auto merge ⚡ label Jan 20, 2021
@ghost ghost merged commit 5424989 into CommunityToolkit:master Jan 21, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior doc-provided ✔️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImageEx causing 'Access violation' exception
5 participants