search

mozilla-mobile / fenix

⚠️ Fenix (Firefox for Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
6.47k stars 1.27k forks source link

[Bug] Top Sites - renaming on the second page not possible #18672

Closed Firecash closed 3 years ago

Firecash commented 3 years ago

Steps to reproduce

1) Capture 8 important pages (or more) for the home page.

2) On the second page (change to the second page with a swipe gesture) of the -important pages-

3) Renaming of the entries is not possible

Expected behavior

The renaming of the entries should also be possible for entries on the second page.

Actual behavior

Entries on the second page of the important pages cannot be renamed.

Device information

In my opinion, the problem devices is independent. The error also occurs on completely different devices. Also available in pre-release versions up to the Nightly version

┆Issue is synchronized with this Jira Task

Mugurell commented 3 years ago

It seems like renaming works - if after renaming a top site the app is restarted, when viewing again the list of top sites we'll see the updated name. Is just that we are not properly updating the TopSites item on the second page.

And this because here we iterate through all top sites and save the index of the renamed top site And here we'll try to update the item at that index. But a top site shown on the second page would have an index between [8, 15] while we can only show 8 items.

So a quick fix is to just use the remainder of item.first % TOP_SITES_PER_PAGE instead of the item index when trying to update a top site.

SoftVision-LorandJanos commented 3 years ago

Hi @Firecash ! Thank you for the report! I tested it on the Latest Release 87.0.0-rc.1 and on the latest Nightly 210329 00:12 (GV89). This issue is reproducible. Device used: Motorola Nexus 6 (Android 7.1.1)

kanish671 commented 3 years ago

I think the same bug exists for Remove option too. I would like to take up the fix for this issue.

jonathanalvares9009 commented 3 years ago

Can I work on this issue?

Mugurell commented 3 years ago

Can I work on this issue?

Sure! Thank you!

Mugurell commented 3 years ago

I think the same bug exists for Remove option too. I would like to take up the fix for this issue.

Sorry, I see you wanted to wanted to work on this and announced it earlier. The ticket is yours if you wanted.

Regarding "Remove" I see removing a top site even from the second page of top sites works. Is this not the case for you. If you see an issue with that option please open a new ticket.

kanish671 commented 3 years ago

Yes, I would like to work on this issue and create a PR.

I am currently using 87.0.0-rc1. I am unable to remove top sites from the second page. I use a OnePlus 7 handset running Android 10. Although, in my local emulator, I am able to remove top sites from the second page without issues. So I guess it has been fixed.

kanish671 commented 3 years ago

I was able to make the changes and get the top sites renaming working.

But this is bringing up another bug which I believe is hidden currently because renaming and subsequent re-rendering doesn't happen. This happens if page 2 contains less than 5 items in the list. The grid snaps to a single line on rename and it stays that way even for page 1.

Page 2

Screenshot 2021-03-30 at 8 52 50 PM

Page 1

Screenshot 2021-03-30 at 8 53 05 PM

Do you want me to try and fix this issue too?

Mugurell commented 3 years ago

That is interesting. Seems like to to fully fix the renaming issue we need this part resolved also otherwise we'd be introducing an even bigger bug. We'll probably need a custom ViewPager that remeasures each page after swiping to it.

Mugurell commented 3 years ago

Measuring each page separately would mean that swiping between top sites pages with different heights (2 rows of topSites and then 1 row) the entire topSites layout would become smaller/bigger and all the other homescreen items "move" in response. Right now having <=4 top sites on the second page still has that page occupying two rows. After the above proposed change the 1st page of top sites would occupy 2 rows and the second page just 1 row.

Asking UX if the top sites layout should have a dynamic height when switching between pages or all pages should have the height of the biggest one. (I think we can "remember" the biggest height and use that in our custom ViewPager even if another page would have a smaller height).

kanish671 commented 3 years ago

Yeah, I too think this is a much bigger bug than renaming not reflecting immediately. Will wait for input from UX before I proceed on this further.

drs12345-SS commented 3 years ago

Can somebody help me to know that what we have to do with this issue. I am not getting any clue how to fix this

Mugurell commented 3 years ago

@drs12345-SS To fix just the renaming issue we could use this - https://github.com/mozilla-mobile/fenix/issues/18672#issuecomment-809307282

Fixing the rename being visible issue will show another issue and based on the investigations from kanish671 we are waiting for UX to confirm what's the expected behavior:

After knowing that we can use a custom ViewPager which remeasures the pages on each swipe - if deciding to use different pages heights or uses a cached height value - if deciding to use the two rows on each page (even if on the second one there are fewer than 4 top sites displayed).

The UX team has a lot on their plate currently, please wait until we get a response.

topotropic commented 3 years ago

When people have more than 8 top sites the layout should stay consistent – so all pages have the height of the biggest one. Thanks!

Mugurell commented 3 years ago

@kanish671 I think we now have all the needed details about how the feature should function. Do you plan to work on this? Need any help?

kanish671 commented 3 years ago

Hi @Mugurell Yes, now we have the details. I would need help in understanding what components would need modification. I would like to try fixing this.

drs12345-SS commented 3 years ago

actually i am new to it but i want to contribute in mozilla so can you please guide me about the bug that what actually it is? .

On Thu, Apr 8, 2021 at 11:20 PM Anish Krishnaswamy @.***> wrote:

Hi @Mugurell https://github.com/Mugurell Yes, now we have the details. I would need help in understanding what components would need modification. I would like to try fixing this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla-mobile/fenix/issues/18672#issuecomment-816019230, or unsubscribe https://github.com/notifications/unsubscribe-auth/ATRPCAMGZJ5IX6OP7D7KL5DTHXUETANCNFSM4Z55OGYA .

kanish671 commented 3 years ago

@drs12345-SS I am actually working on this and contributing to fix the bug myself. The original bug was that renaming of top sites in page 2 did not reflect immediately. I made changes to fix that, which uncovered another bug. The new bug description is mentioned here

kanish671 commented 3 years ago

@Mugurell Here is my thought on how we can set the height: If we are rendering page 2, we know for sure that page 1 would be full. So I think we can set the height of the view as that of two rows whenever there are more than 4 items in top sites list. I am quite new to android programming, so would need a bit of guidance on how to achieve this.

Mugurell commented 3 years ago

Hello @kanish671 Sorry I haven't responded earlier. It might be that the later changes are not trivial. Other than https://github.com/mozilla-mobile/fenix/issues/18672#issuecomment-810808051 and https://github.com/mozilla-mobile/fenix/issues/18672#issuecomment-810837558 I don't have other insights. You can open a PR with your changes and I'll look into resolving the viewpager height part.

kanish671 commented 3 years ago

Sure @Mugurell I will raise a PR for fixing the renaming bug

sflorean commented 3 years ago

Verified as fixed on Nightly 5/19 with Samsung Note 10 (Android 11).