Slow loading on the "To Label" screen + minor fit-and-finish #662
Comments
shankari
changed the title
This behavior existed even before the "to label" changes, but would typically not get triggered, since we would load all trips by default. So the chances that we would not add any elements to the list for each call to |
|
Checking server calls from the user who reported this, I find one call to As the user indicated, this is followed by a call to the diary |
|
If I am right, the steps to reproduce should be:
Yup, reproduced using my test phone, which has no unlabeled trips. @GabrielKS, I am sending you the token privately if you want to use it to test. It doesn't take one minute for me, but that is because I have a lot fewer trips than the user who reported this. Here are the calls from the test phone |
|
@GabrielKS please outline your proposed fix here since I don't think it is trivial. LMK if you want me to design a fix instead. |
|
If the problem is as you've described, then I agree the fix will take some thought. I don't think I'll have time to design and implement a fix by Monday, so if we're still looking to deploy quickly it might be best if you took this one. |
|
Given the tight timeframe, I want a relatively simple fix, not one that ideally does not modify the data retrieval process. For example, one potential fix would be to query the server for the most recent "to label" trip, and start retrieving from that point. But that would some significant API redesign since the current retrieval code depends only on the object keys, and not the other fields of the object. Should we support retrieval by other keys? Which subset of keys? Or should we support all keys? Is the server then turning into a proxy for mongodb? Also, if we use that approach, then the filters will also need to be rewritten. If a user chooses "All Trips", for example, we need to retrieve all trips from now to the first "to label" trip since they won't be in memory any more. While this may be the long-term solution, it is a significant rewrite that cannot be completed in a day. But looking at the underlying problem, the real problem is that we keep requesting updates all the way to the oldest trip without giving the user a chance to weigh in, like they could do when we were loading all trips. So even if the user knows that they don't have any more "to label" trips, or don't want to deal with them now, they can't do anything to stop the retrieval. In other words, we are "scrolling", whether the user wants it or not. So an alternate fix is to just stop doing that. After we retrieve the first batch, even if it is empty, don't keep loading. Instead display a message saying something like "no labels found until X, scroll to load more". This will require changes to the scrolling code that @GabrielKS wrote, but hopefully it is a simple question of adding additional state around the code which tracks when the scroll status is at the top, and calls the reload function. |
|
Here's where we make the first call to readDataFromServer And then we make a server call We get another scroll message but ignore it since we are still processing the existing call We received 5 more messages And then broadcast that we are done. We get another scroll event so we load again and get a batch of size 3 But then we get a scroll message again, and get a batch of size 0 And then we get a scroll message again, and get a batch of size 2 Until we finally end. |
|
I wonder if we just need to fix this with standard debouncing logic. Once we have called the |
|
Setting a debouncing timeout of 100 ms seems to still be problematic. |
|
But a debouncing timeout of 5 seconds seems fine |
|
Now that we have a working workaround, since the real problem is multiple calls to did not work |
|
Tried to scroll down by a little so we wouldn't trigger the scroll behavior again. didn't work |
|
Added some additional logging to see how the top and the threshold change and found some interesting results. After the initial load, we scroll up we read data from the server and then we "adjust" by scrolling further The adjustment Generates additional scroll events which move things around And so on These additional scrolls seem to be the reason why we get multiple calls to the server. I have an intuition that they may also be responsible for the misordered trips, although I can't see any concrete use case of how that would happen. I have a workaround in place, but am going to dig deeper into the scrolling for now to see if I can fix it at that point. |
|
I found some examples of "reverse infinite scroll using angular JS" There are two libraries that claim to work for this. Maybe we should try one of them... |
|
As a first step, I just commented out the scrolling code in the callback. And visually, everything seemed to work fine even for:
@GabrielKS do you remember why you had to add this code to scroll to the bottom and then scroll back to the top? If we are already at the top, it seems like, at least in the case where we do get new entries added above, it should be fine to just stay there. |
|
While loading data for the user who originally reported the problem, though, we run into the issue that opening the popover to fill in the values generates a subsidiary scroll event, which:
I saw similar behavior with the Given that the scrolling code is so finicky, I might just disable it and switch to a "load more" button instead. Popup behavior with single unlabeled tripWe read the initial data and broadcast infiniteScrollComplete We read a second time because we are still at the top, and we broadcast it again Then we try to open the popover, which generates a scroll event And what's worse, the data re-read resets the currently edited value, which causes an error while setting the value from the opened popup. |
My thinking was that if we scroll up to the top to load more trips, the newly loaded trips should appear to load "above" the current scroll position, so the same trips appear on the screen after the load completes and the user can scroll up to see the newly loaded ones. This behavior is shown in e-mission/e-mission-phone#769, and it was what we had back when we infinite scrolled downwards. Back then, the behavior would happen automatically because loading new trips would not change the position of existing trips on the page, but now when new trips load they get inserted in the page before existing trips and push the existing trips down. Thus, we have to automatically scroll back down to the trips we were looking at before. The One option I did not try is to insert some placeholder element in the DOM at the current scroll position before loading new trips and then use |
|
I can confirm that going back to the original "upwards scrolling" implementation causes multiple calls in quick succession. At that point, though, there was no additional scrolling, so I was able to set the values. Log of the multiple calls with the original implementation. |
|
I can also confirm that this behavior is not related to the scroll order, but rather, to the selection of a filtered screen as the default view. We did not see this behavior when the default was "AllTrips" because there would typically be some additional entries loaded. I reverted to the change before the scroll order I then reloaded the label screen for the user with no "to label" entries. I got the same behavior. Log of the multiple calls with the "infinite scroll from the bottom" implementation |
|
I was able to reproduce the duplicated trips issue while testing against dee142999681fb9378eedf3679e4bcec0681a715 |
|
Was able to reproduce it again! Not sure what race condition is causing the second read. Note that the first call overlaps with the initial calls to get the user inputs, which may be a clue... Logs for the race conditionSetup Read trips from But then we again read trips from After that, we read trips from |
|
Given that this appears to be a race condition with multiple calls to |
The auto-load on scroll feature, so common to messaging and chat apps is actually fairly complicated to implement correctly in the case in which we display a filtered list. In particular: - the additional scrolling required to maintain "place" in the list triggers multiple "scroll" calls even when the user is not doing anything, which leads to multiple calls to the server. - if new entries are not added to the list after the server call, then the condition that triggered the server call (top < threshold) persists, leading to multiple calls again. See e-mission/e-mission-docs#662 This is an inherent limitation of using auto-load on scroll for a filtered list and also happens with the standard ionic component, e-mission/e-mission-docs#662 (comment) So instead of butting my head against the wall getting auto-load to work, I switched to manual load, similar to the diary screen. e-mission#769 As a bonus, this also helps with e-mission/e-mission-docs#659 It indicates how many of the trips are displayed in **this filter**, should help to clarify questions around "missing trips" and allow users to explore other tabs as necessary. Testing done: - loaded data for a user with a single trip - labeled that trip so the user had no trips - loaded data for a user with multiple trips In all cases, used the button to scroll all the way to the oldest trip. Did not encounter any issues.
This fixes one of the minor fit-and-finish issues "If you go to "All Trips", you are at the top, showing the oldest trips, and have to scroll all the way to the bottom" from e-mission/e-mission-docs#662
|
I'm now going to tackle The obvious solution is to add a timeout before marking the trip for removal. But we currently remove the trips by calling We can fix this by either:
Note that even if we do introduce a flag in the entry, we will still have to either:
If we don't, when the user edits one trip multiple times, we will schedule multiple deferred calls for the same item, and may end up deleting too close to the second edit. Pros and cons:
|
|
Actually, Let's try that first. |
|
well, that was easy e-mission/e-mission-phone#787
|
|
Current status of these fit and finish fixes is:
|
|
Not sure that the three dots is a showstopper. I also can't reproduce it in the emulator. |
|
I had some users test out the delayed deleted today morning, with a specific focus on whether the delay was confusing. One of them preferred to have the trips disappear after a button click - i.e. the user would select the three popup labels and then click on the green check mark to confirm correctness and cause the trip to disappear. That is a significant difference from the planned "auto-disappearing" functionality, and it actually increases the number of clicks. So I don't think we should go down that route for now. But it is something to consider for a future change if we get a lot of confusion and compliants with the current approach. I also wonder whether we should lower the delay to 30 secs. When I saw my kids using the screen, by 1 minute, they had forgotten about the labeled trip and were kind of surprised that it went away. @GabrielKS what do you think? |
|
I wouldn't mind 30 seconds. I agree that 1 minute is kind of a long time. |
|
Adjusted in e-mission/e-mission-phone#790
|
The auto-load on scroll feature, so common to messaging and chat apps is actually fairly complicated to implement correctly in the case in which we display a filtered list. In particular: - the additional scrolling required to maintain "place" in the list triggers multiple "scroll" calls even when the user is not doing anything, which leads to multiple calls to the server. - if new entries are not added to the list after the server call, then the condition that triggered the server call (top < threshold) persists, leading to multiple calls again. See e-mission/e-mission-docs#662 This is an inherent limitation of using auto-load on scroll for a filtered list and also happens with the standard ionic component, e-mission/e-mission-docs#662 (comment) So instead of butting my head against the wall getting auto-load to work, I switched to manual load, similar to the diary screen. e-mission#769 As a bonus, this also helps with e-mission/e-mission-docs#659 It indicates how many of the trips are displayed in **this filter**, should help to clarify questions around "missing trips" and allow users to explore other tabs as necessary. Testing done: - loaded data for a user with a single trip - labeled that trip so the user had no trips - loaded data for a user with multiple trips In all cases, used the button to scroll all the way to the oldest trip. Did not encounter any issues.
This fixes one of the minor fit-and-finish issues "If you go to "All Trips", you are at the top, showing the oldest trips, and have to scroll all the way to the bottom" from e-mission/e-mission-docs#662
@GabrielKS just writing these down so you can finish them off.
The text was updated successfully, but these errors were encountered: