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

Discover random photos #17

Draft
wants to merge 2 commits into
base: preview
Choose a base branch
from
Draft

Discover random photos #17

wants to merge 2 commits into from

Conversation

kvalev
Copy link
Owner

@kvalev kvalev commented Sep 10, 2021

No description provided.

@kvalev kvalev force-pushed the kvv-discover-random branch from 2ca690d to d0b1b3e Compare September 10, 2021 18:05
@kvalev kvalev added the enhancement New feature or request label Nov 19, 2021
@rickysarraf
Copy link

Hello @kvalev

Wondering if you've noticed performance issues with pages Discover Random and Discover On This Day. Usually, page scrolls are terribly slow on them. And it is only on these pages. Stock pages, like /browse, are very performant.

@kvalev
Copy link
Owner Author

kvalev commented Mar 26, 2022

Hey @rickysarraf,

no, not really, but to be honest I am not using them very often. Discover Random has not been merged yet, partly because in /browse you can already sort by random, so the page feels redundant. When do you notice performance problems? Is there a certain threshold (number of photos or number of years in case of Discover On This Day)?

@rickysarraf
Copy link

@kvalev I must have cherry-picked the MR for Discover Random.

The performance problem is with both, On THis Day and Random, but only under the Discover tab.

I noticed the problem right from the beginning when it wants to render the page. Compared to pages like /browse, there's a noticeable difference. Then, also, the scrolling is slow, as and when it scrolls and fetches more data.

There isn't any large set of data I think. For example, On this Day, already narrows it down to results from one particular day/date.

I'm not very savvy on web technologies but if you point me to what all I should do to help provide you with more information, I'll be glad to do that.

@rickysarraf
Copy link

I went and tried to do some checks. First of all, sorry, the issue is only with Discover On THis Day.

I used Chromium to do my first debugging. Thankfully, Chromium has a Mobile View, wherein I can set the device display type to be of Mobile View. I did so because I, only now, realized that the problem was prevalent on Mobile view. The problem seems that when you (try to) do a scroll/touch on the page, it aggressively tries to do some (pre) loading in the background.

On looking in Chromium debug tool, I get the following:
Screenshot_20220327_211248

@rickysarraf
Copy link

rickysarraf commented Mar 27, 2022

Firefox too has some Developer Tools. And I realized that Firefox is facing the problem in Desktop View mode too. Here's an interesting finding on the Firefox side.

  • Load the On This Day page
  • Now, a very slight scroll results in more fetches being triggered.
  • Lots and lots of GET method on files tile_500
  • The total count of pictures On This Day/Date is just 12

image

@kvalev
Copy link
Owner Author

kvalev commented Mar 27, 2022

Thanks a bunch for the pointers! I will taka a look ASAP. It is very weird that this happens even when there are only 12 photos. I tested in the past few days and I had significantly more than 12, and the UI was quite snappy. I tried it both with Firefox as well as the chrome-based mobile app and no issues so far. Now that you posted your benchmarks, I will check mine and will let you know what I figured out.

@fly-man-
Copy link

[Discover on this day]

I have noticed that when I open up the [On this day] tab that the pictures aren't in chronological order. They're going all through the years

Example: 2012, 2019, 2016, 2015

Where I would expect the oldest one to be at the top and then reaching up to the latest.

@rickysarraf
Copy link

[Discover on this day]

I have noticed that when I open up the [On this day] tab that the pictures aren't in chronological order. They're going all through the years

Example: 2012, 2019, 2016, 2015

Where I would expect the oldest one to be at the top and then reaching up to the latest.

I never paid attention to the sorting order. But now that you've reported @fly-man- I have picked up this MR in my local test build.

@kvalev The order on my machine, is still from latest to oldest. Whereas what is reported here, as expected, to have the oldest listed first.

@kvalev
Copy link
Owner Author

kvalev commented Nov 4, 2022

Thanks @rickysarraf for testing! Do you see any performance problems or improvements?

Re sorting - I personally think that from newest to oldest is a more logical sort order (hence the change in the PR), but if there is a strong pull for the other way around I can also make the order configurable.

@rickysarraf
Copy link

Thanks @rickysarraf for testing! Do you see any performance problems or improvements?

I did not encounter any performance issues. But that is because I don't have enough data to render a longer list for On This Day.

Re sorting - I personally think that from newest to oldest is a more logical sort order (hence the change in the PR), but if there is a strong pull for the other way around I can also make the order configurable.

I agree. I think it is logical to have the newest first, as the default. Having it configurable can make it much better.

@rickysarraf
Copy link

By the way, @kvalev, The initial performance issues that I had faced, for which I reported this issue, is now resolved. My guess is that the performance enhancements that Heiko Mathes added, play a very important role here.

@kvalev
Copy link
Owner Author

kvalev commented May 12, 2023

I think there isn't much need for this feature, as it is possible to sort by random on the search page, which makes this Discover random page obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants