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.

Sign up for GitHub

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

Jump to bottom

Counter values don't get recalculated anymore after the whole list is loaded. #4417

Closed
7 tasks done
bedhub opened this issue Jul 29, 2022 · 0 comments · Fixed by #4512
Closed
7 tasks done

Counter values don't get recalculated anymore after the whole list is loaded. #4417

bedhub opened this issue Jul 29, 2022 · 0 comments · Fixed by #4512
Labels
bug broken functionality, usability problems, unexpected errors state:done meets our definition of done state:tested We tested it and are about to release it
Milestone
3.100.2

Comments

@bedhub
Copy link
Contributor

bedhub commented Jul 29, 2022 •
edited by charlag

  1. Have wrong folder counter
  2. Load all mails of that counter
  3. Error -> Counter is not updated

Expect: Folder counter should be synchronized.

Hint:

  • it may be possible to override counter values with the admin client to get to the broken state
  • check if the count gets recalculated at all at the intended time
  • try breaking something in the client to get to an invalid count

Test Notes

you'll have to build a client that can set invalid mail counts (maybe expose the method that corrects it in MailListView.ts globally and build the client against the test system, call the method for the correct listId with an incorrect count and then check that the issue is fixed in the client under test.

  • mail lists with a multiple of 100 mails and an incorrect mail counter are corrected when scrolled to the end
  • mail list not containing a multiple of 100 mails and and incorrect mail counter are corrected when scrolled to the end
  • mail lists with correct counters remain correct when scrolled to the end
  • previous points work with the inbox
  • previous points work with the trash
  • previous points work with a custom folder
  • none of these lead to a neverending cycle of trying to fix and then re-dispatching (check console for "fixing up" or "no fixup needed" messages that signal the end of the cycle)
@bedhub bedhub added the bug broken functionality, usability problems, unexpected errors label Jul 29, 2022
@ganthern ganthern changed the title Fix counter values if whole list is loaded is broken Counter values don't get recalculated anymore after the whole list is loaded. Aug 4, 2022
@paw-hub paw-hub assigned paw-hub and nif-tutao and unassigned paw-hub and nif-tutao Aug 11, 2022
@ganthern ganthern self-assigned this Aug 16, 2022
ganthern added a commit that referenced this issue Aug 17, 2022
@ganthern
fix unread mail counters not being corrected on full list load
58c8f6d 
previously, list would have a callback argument that got called when it
was fully loaded, which is where fixCounters was previously called.
With the removal of that argument in d7a14a, it was moved to the fetch
argument and called asynchronously in parallel.
It contains some checks to make sure we're setting the right value on
the counter, one of which caused us to almost always bail out after the
sequencing change:

* fetch
* get new items
* check if it's completely loaded and dispatch fixCounter if it is
* immediately return new items to be inserted in the list.
* run fixCounter, which would notice that the new items were inserted
  after dispatch and bail out because the list changed

With this commit, we add a timestamp of the last element push/removal
as a marker for list changes (better than checking the length).
It would be better to create some MonitoredArray class that can track
modifications instead of tracking the array and the timestamp as
separate fields, but this is the easiest and certainly more performant
than proxying array accesses.

With this commit, the mailListView will also re-dispatch the fixCounter
call if the list changed so we don't miss the chance to fix the counter.
There's a potential issue where the "fully loaded" state reverts between
the first and second call, but I couldn't figure out when and ifthis
might happen.

fix #4417
@ganthern ganthern mentioned this issue Aug 17, 2022
Merged
ganthern added a commit that referenced this issue Aug 17, 2022
@ganthern
fix unread mail counters not being corrected on full list load
b319e4b 
previously, list would have a callback argument that got called when it
was fully loaded, which is where fixCounters was previously called.
With the removal of that argument in d7a14a, it was moved to the fetch
argument and called asynchronously in parallel.
It contains some checks to make sure we're setting the right value on
the counter, one of which caused us to almost always bail out after the
sequencing change:

* fetch
* get new items
* check if it's completely loaded and dispatch fixCounter if it is
* immediately return new items to be inserted in the list.
* run fixCounter, which would notice that the new items were inserted
  after dispatch and bail out because the list changed

With this commit, we add a timestamp of the last element push/removal
as a marker for list changes (better than checking the length).
It would be better to create some MonitoredArray class that can track
modifications instead of tracking the array and the timestamp as
separate fields, but this is the easiest and certainly more performant
than proxying array accesses.

With this commit, the mailListView will also re-dispatch the fixCounter
call if the list changed so we don't miss the chance to fix the counter.
There's a potential issue where the "fully loaded" state reverts between
the first and second call, but I couldn't figure out when and ifthis
might happen.

fix #4417
ganthern added a commit that referenced this issue Aug 18, 2022
@ganthern @charlag
fix unread mail counters not being corrected on full list load
90ab925 
previously, list would have a callback argument that got called when it
was fully loaded, which is where fixCounters was previously called.
With the removal of that argument in d7a14a, it was moved to the fetch
argument and called asynchronously in parallel.
It contains some checks to make sure we're setting the right value on
the counter, one of which caused us to almost always bail out after the
sequencing change:

* fetch
* get new items
* check if it's completely loaded and dispatch fixCounter if it is
* immediately return new items to be inserted in the list.
* run fixCounter, which would notice that the new items were inserted
  after dispatch and bail out because the list changed

With this commit, we add a marker of the last element
push/removal/replacement (better than checking the length).
It would be better to create some MonitoredArray class that can track
modifications instead of tracking the array and the marker as
separate fields, but this is the easiest and certainly more performant
than proxying array accesses.

With this commit, the mailListView will also re-dispatch the fixCounter
call if the list changed so we don't miss the chance to fix the counter.
There's a potential issue where the "fully loaded" state reverts between
the first and second call, but I couldn't figure out when and if this
might happen.

fix #4417

Co-authored-by: ivk <ivk@tutao.de>
@bedhub bedhub added this to the 3.100.x milestone Sep 19, 2022
@charlag charlag modified the milestones: 3.100.x, 3.100.1 Sep 20, 2022
charlag added a commit that referenced this issue Sep 20, 2022
@ganthern @charlag
fix unread mail counters not being corrected on full list load
60fdd2f 
previously, list would have a callback argument that got called when it
was fully loaded, which is where fixCounters was previously called.
With the removal of that argument in d7a14a, it was moved to the fetch
argument and called asynchronously in parallel.
It contains some checks to make sure we're setting the right value on
the counter, one of which caused us to almost always bail out after the
sequencing change:

* fetch
* get new items
* check if it's completely loaded and dispatch fixCounter if it is
* immediately return new items to be inserted in the list.
* run fixCounter, which would notice that the new items were inserted
  after dispatch and bail out because the list changed

With this commit, we add a marker of the last element
push/removal/replacement (better than checking the length).
It would be better to create some MonitoredArray class that can track
modifications instead of tracking the array and the marker as
separate fields, but this is the easiest and certainly more performant
than proxying array accesses.

With this commit, the mailListView will also re-dispatch the fixCounter
call if the list changed so we don't miss the chance to fix the counter.
There's a potential issue where the "fully loaded" state reverts between
the first and second call, but I couldn't figure out when and if this
might happen.

fix #4417

Co-authored-by: ivk <ivk@tutao.de>
@charlag charlag self-assigned this Sep 20, 2022
@charlag charlag added the state:tested We tested it and are about to release it label Sep 20, 2022
@charlag charlag removed their assignment Sep 20, 2022
@charlag charlag mentioned this issue Sep 21, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug broken functionality, usability problems, unexpected errors state:done meets our definition of done state:tested We tested it and are about to release it
Projects
None yet
Milestone
3.100.2
Development

Successfully merging a pull request may close this issue.

fix wrong unread counter not being fixed tutao/tutanota
5 participants
@charlag @bedhub @ganthern @paw-hub @nif-tutao