Revisit and unify user input label handling #674
Comments
|
I have been thinking for a while that, instead of having separate we may want to have a data structure with key I can see three possible advantages with this change:
|
|
I'm going to start with making the changes on the client side and refactoring the code accordingly. If it works, we can change the server and write the migration script. But let's defer that until we know that the client side changes look good. Since the client-side changes for existing platforms may not be pushed for a while, we will have to maintain some backwards compat code on the server for a couple of months. |
|
@shankari : thanks for the force sync commit :-) we have at least one Android user obliged to force sync manually from time to time because the data doesn't seem to make it to the server in the background.
thanks again! We send this manual labels to the personal cloud of the beta-testers and although not using this data yet, we intend to. |
|
In our case, we have multiple modes and purposes. So we cannot use either of them. Right now, we take the advantage of the
The fact that the proposed I would like to give you of the data structure we are using now. It might spark some idea for your new design. Here is the const data = {
start_ts: number, // for trip reference
end_ts: number, // for trip reference
uuid: string, // for user reference
timestamp: number, // for sorting purpose
label: string, // for button label
name: string, // enketo survey name (for filtering)
version: string, // enketo survey version (for filtering and compatibility resolution)
xmlResponse: string, // enketo survey response xml string
}In our case, the thing we do not wanted to touch the most is adding new key to the server. So generalize user input into one data type bring us the great benefit. |
Let me know the hack fixes the issue for the user. I can then merge it, and think about the longer-term native fix as well. |
|
@atton16 thanks for the example! You can, of course, use any fields within the object that you like. Having said that, you don't need the UUID, since all entries include the UUID by default - the entry structure is If the The rest of the fields make sense. I would suggest that you should convert the |
I could convert Edit: |
Until it is pushed, you don't need the UUID, since you are only dealing with data from one user on the phone. And we do have metadata for the entries retrieved on the phone - that's why you have to use As another example, we actually use the metadata in the UnifiedDataLoader to find duplicate elements between the locally retrieved elements and the remotely retrieved elements. |
Thank you for the clarification. I might be able to get rid of it in our survey and using the provided fields. |
|
At this point, we have the label code largely pulled out into its own modular element. There are currently two main aspects that remain in These are:
|
|
The obvious design decision for the first choice is to populate the value in the directive.
However And since the directive works on the current trip by default, this is a problem. Some solutions we can rule out:
So we have to pass in the next trip as well (for backward compatibility). We are currently iterating through the trips using
Options (2) and (3) are very similar, and are the closest to the current codebase. Let's go ahead and use (2) because it feels a little nicer, and there are less of an input to pass in. |
+ To allow us to move user input labeling into the directive as well (e-mission/e-mission-docs#674 (comment)) This was a little tricky because it turns out that when the directive is created, `tripgj` is undefined. It then gets updated through the digest process. The really weird thing is that when we log the `scope` object directly, it looks like it also gets updated with the digest. So we ended up with logs like ``` Scope isolateBindings: {inputs: {…}, tripgj: {…}, unifiedConfirmsResults: {…}, inputParams: {…}} inputParams: {MODE: {…}, PURPOSE: {…}} inputs: (2) ["MODE", "PURPOSE"] tripgj: {data: {…}, start_place: {…}, style: ƒ, onEachFeature: ƒ, pointToLayer: ƒ, …} unifiedConfirmsResults: {MODE: Array(60), PURPOSE: Array(63)} userInputDetails: (2) [{…}, {…}]__proto__: Object unifiedresults [object Object] scope trip information is undefined ``` which was driving me crazy. After adding the `$watch` callback, we can now fill the inputs correctly `` the trip binding has changed from [object Object] to new value Checking to fill user inputs for [object Object] potentialCandidates are 2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) shared_ride logged at 1632745829.985,2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) drove_alone logged at 1632745826.26,2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) shared_ride logged at 1602636485.509,2016-08-04T13:03:51-07:00(1470341031.235) -> 2016-08-04T13:35:12-07:00(1470342912) shared_ride logged at 1602636496.041:undefined ``` + Check in the multi-label-ui for the first time. Better late than never!
|
The design for the second choice is more complicated. I re-added the listener to the element passed in to the controller, and it doesn't work - there are no matches for " I'm also a bit confused about what we use this for, and how it works because, even in the original controller, the output is just |
|
So the initial load doesn't initialize anything, but we do have entries before we start matching inputs Where are these changed and if we don't rely on the callback to populate them, can we remove it? |
|
if the callback is removed, we don't have any labels so it is clearly needed.
|
|
ah, we do load the data, we just do so asynchronously. Note also that it is a bit sub-optimal to read the options for each directive separately. But we don't want to keep this code in this list since it is not relevant for the purely passive case, or in the case of other labels such as surveys. What we really need is a shared datastructure across the directives, but outside the list. Shared datastructures in angular are (IIRC) stored in services. We have a service (or rather, a factory, which is similar). Can we move this code into ( |
|
I think we can, but we have to think carefully about the timing. Concretely, we have two independent threads:
This was fairly wonky even before, because we launched the JSON options load on the view loaded event, and populated the inputs while handling the trip geojson. The first operation is short and the second is long, so the timing almost always worked. But if the first operation slows down for some reason, the code will break. Verified this by adding a 60 sec timeout to the load function We got the following output. Clearly, due to the 60 sec delay in loading the trip options, we error out while filling them in, and we don't see the user inputs.
|
|
It is not too terrible to keep the timing as-is, since the first call involves a local read, and the second call involves a network read, so the chances of out-of-order execution are vanishingly small. And when they do occur, they can easily be fixed by reloading. But since we are here anyway, let's see if we can fix this small race condition as well. |
|
what I really need is a singleton design pattern. With that, the |
|
After adding some logs: We get the following logs: |
|
Since the service is a singleton, we should be able to have a single We need to know that the data is present before we call |
Fortunately, all factories and services are already singletons. e-mission/e-mission-docs#674 (comment) The property is read asynchronously, though, so we have the data that we want to read stored as a promise. The first time we access the field, the async operation completes and the promise completes. In subsequent calls, the promise is fulfilled and will return immediately. This allows us to retain the desired singleton behavior even for an asynchronously read data source. Testing done: Promise executes first and only time ``` Starting promise execution with {} Promise list (2) [Promise, Promise] Read all inputParams, resolving with {} controller DiaryListCtrl called ``` It is then available later but without re-reading or delay ``` While populating inputs, inputParams {MODE: {…}, PURPOSE: {…}} While populating inputs, inputParams {MODE: {…}, PURPOSE: {…}} ``` Introduced a timeout while reading the config ``` diff --git a/www/js/tripconfirm/trip-confirm-services.js b/www/js/tripconfirm/trip-confirm-services.js index 8089362e..291184fa 100644 --- a/www/js/tripconfirm/trip-confirm-services.js +++ b/www/js/tripconfirm/trip-confirm-services.js @@ -1,5 +1,5 @@ angular.module('emission.tripconfirm.services', ['ionic', 'emission.i18n.utils', "emission.plugin.logger"]) -.factory("ConfirmHelper", function($http, $ionicPopup, $translate, i18nUtils, Logger) { +.factory("ConfirmHelper", function($http, $ionicPopup, $translate, i18nUtils, Logger, $timeout) { var ch = {}; ch.INPUTS = ["MODE", "PURPOSE"] ch.inputDetails = { @@ -139,8 +139,10 @@ angular.module('emission.tripconfirm.services', ['ionic', 'emission.i18n.utils', ch.INPUTS.forEach(function(item, index) { inputParams[item] = omObjList[index]; })); - console.log("Read all inputParams, resolving with ", inputParams); - resolve(inputParams); + $timeout(() => { + console.log("Read all inputParams, resolving with ", inputParams); + resolve(inputParams) + }, 60000); }); ``` The promise reading started first ``` Starting promise execution with {} Promise list (2) [Promise, Promise] controller DiaryListCtrl called ``` Then, everything was filled in ``` DEBUG:section 2016-08-04T14:18:35.840464-10:00 -> 2016-08-04T14:34:12.571000-10:00 bound incident addition ``` Several minutes after that, the promise resolved ``` Read all inputParams, resolving with {MODE: {…}, PURPOSE: {…}} Checking to fill user inputs for 10:03 AM -> 10:35 AM While populating inputs, inputParams {MODE: {…}, PURPOSE: {…}} Set MODE {"text":"Shared Ride","value":"shared_ride"} for trip id "5f8646cde070a9164325d554" ``` And the labels were automagically filled in and green. yay for promises!
|
At this point (with e-mission/e-mission-phone@c9621c3), we have removed all vestiges of the Now let's try to use the same or a similar directive in the infinite scroll list. This should have the side effect that any labeling improvements to the list view will also show up in the diary. |
|
The changes between the new directive and the infinite scroll list are minor. The primary changes and their resolution are:
wrt the So it is not currently used anywhere. But if we do ever get to the point where we fix the walkthrough "the right way", we will probably want to restore the pointers, so we will keep the class around. |
|
The padding style for the enclosing There isn't much of an explanation of why we needed to remove the right/left padding. @GabrielKS, do you remember? Looks like the top margin was always zero from when we copied the information over to create the infinite scroll list (e-mission/e-mission-phone@85a56ba) I really don't like tweaking CSS values, but it looks like @GabrielKS intentionally removed the padding, so let's just keep that version for now. |
And using the new directive in both places Concretely: - unify the HTML code as per e-mission/e-mission-docs#674 (comment) and e-mission/e-mission-docs#674 (comment) - replace the buttons in the label screen with the new directive - modify the directive slightly to skip `populateInputFromTimeline` if no input label list is passed in. Testing done: - Diary screen shows the trips with green labels - Label screen with the "All Trips" filter shows the trips with green labels
|
Which lines are you referring to? I messed with margins and padding a fair amount; sometimes it was to streamline unnecessary or out-of-place code, sometimes it was for greater cross-platform reliability, sometimes it was just to make things look better. |
|
As we integrate the label screen functionality into the directive, one fairly major question relates to the "one-click" labeling of trips. Should that be within the directive or not?
Options:
If we can figure out a way to link the two controllers, it seems like two separate directives would really be the best option. It is more modular, and it also addresses the absolute positioning issue. |
Move all the labeling code from the infinite scroll list to the directive
Concretely:
- remove the input detail initialization
- move out the code which initializes the inputs and replace it with the
nextTrip link
- move the manual input population code as well. this is now a separate
implementation from the diary initialization code; we will need to unify it later
- move related functions to the directive:
- `populateInput`,
- `populateManualInputs`,
- `updateVisibilityAfterDelay`,
- `updateTripProperties`,
- `inferFinalLabels`,
- `updateVerifiability`
- delete functions that were already copied from the diary, with minor
modifications as necessary:
- create `$scope.popovers`,
- `$scope.openPopover`
- `$scope.choose`
- `closePopover`
- `$scope.verifyTrip`
also copied `trip.properties.*_ts` into `trip.*_ts` so we can have the same
implementation for both data structures (e-mission/e-mission-docs#674 (comment))
In addition, in the directive, we do the following:
- add functions to find the view element, its state and scope.
- We will use this state to indicate which view we are working with, given
that we plan to support labeling assist in the diary view as well (e-mission/e-mission-docs#674 (comment))
- We will use the scope to callback to the view and filter the trips accordingly (e-mission/e-mission-docs#674 (comment))
This almost works.
The one pending issue is that of filtering to set `$scope.displayTrips`. In
`$scope.setupInfScroll`, we read entries from the server
(`$scope.readDataFromServer`), which sets `$scope.data.allTrips`. It then calls
`$scope.recomputeDisplayTrip` which uses the userInputs to decide which trips
should be in `$scope.displayTrips`.
The problem is that we set the userInputs in the directive. However, the
directives are not executed until they are displayed. So until we compute which
trips go into `displayTrips`, we will not execute the directives, which means
that we will not have the fields we need to compute the directives. We need to
resolve this circular dependency.
|
One final issue:
This is however, really hard to fix. We cannot rely on any directive-level code because the directives will not be initialized unless we set In other words |
|
Another issue is that there is already a dependency from the view controller on the infinite scroll filter, and the filter depends on the structure of the labels. While fixing this, we should fix that dependency if possible as well. |
Inject the filtering factory dynamically Remove the static dependency Initialize the selected filters from the factory This addresses e-mission/e-mission-docs#674 (comment)
Pull out all the populateXXX and recalculateXXX functionality into a separate service so it can be invoked from multiple controllers. This is consistent with e-mission/e-mission-docs#674 (comment) We load the service dynamically to reduce the hardcoded dependency issue. We retain a couple of simple wrappers e.g. `fillUserInputsGeojson` in the controller so that we can invoke it from `$watch` more easily and we can call `$scope.$apply` to update the visualization. Remove references to `scope.manualResultMap` from the controller since the expanded values will be pre-populated. Pass the viewScope around as needed to support the callback after the update. Add a new NOP callback to the diary view to match the desired external interface. Add blank inferred label datastructures for the diary pending a bigger restructuring that will send inferred labels to as part of the geojson.
|
Three final cleanup issues:
I will do the last in a separate PR that does not involve any code changes. |
|
It turns out that the directive This means that actually invoking code within the And we need to initialize the service and precompute values for the label screen anyway. So we remove the |
…e `$watch` Add a simple wrapper (`populateInputsDummyInferences`) to the service to iterate over all the inputs and populate them, similar to the existing `populateInputsAndInferences`. This is consistent with e-mission/e-mission-docs#674 (comment) Dynamically inject the service in the diary as well Call the new method while filling in other fields Move the code to try and unify the trip structure to the new function as well Testing done: Scrolled through the diary and list views, do not see any performance degradation
|
At this point, there is little duplication, and we have cleaned up a bunch of the old code. Next step is to actually send the inferred labels over as part of the geojson for a particular day. This requires a server-side change. |
|
wrt e-mission/e-mission-phone#799 (comment), we should be done. Users can simply change from to and everything will work. But we are not a pure framework, we are also a "platform" that has examples of using all these modules. Changing the implementation by changing the code also results in multiple incompatible branches, which are a pain to maintain. So ideally, we would have several of these changes "configurable" through a config file instead of requiring HTML/JS changes. Technical partners would, of course, be open to making whatever changes they wanted. So I experimented briefly with creating config options configuring the controller and then changing the tag to Unfortunately, that doesn't work since the element tag is treated as a string.
|
|
I tried some other dynamic matching options: Attribute matchingDidn't work because it wasn't expanded properly
Class matchingDidn't work because of lack of matching, even though I changed the directive config to Class matching with hardcoded valueWORKS!! |
|
Double checking this once more because this is so crazy:
The only explanatation is that the binding happens before the scope expansion. I will have to probably create a template for the survey and use |
- Create a "Config" in the top-level survey module for the multi-label survey
- Switch the list views to use the configurations to figure the service and filters
- Switch the HTML to use the configurations to the extent possible. This extent
is the linkedtag attribute of the `verifycheck` element. Alas, we can't
appear to set the element directive tag name directly (see )
e-mission/e-mission-docs#674 (comment)
to
e-mission/e-mission-docs#674 (comment)
+ move the one-click `verifycheck` button outside of `multilabel` since it is explicitly designed for use with other kinds of surveys as well
|
For the record, I also tried to create a new directive The hope was to use I was not able to get it to work:
|
|
Finally, $compile just seems to be completely broken when I try to use it Outputs I don't know what |
|
Ah but hardcoded Doesn't seem to work in the logs but does work in the DOM
|
|
That doesn't display anything because there's no trip attribute, so let's copy over the attributes and HTML, similar to https://stackoverflow.com/a/38410626/4040267 Through dint of unremitting effort and some gnarly inquiries into DOM attributes, I finally figured out This actually does generate the correct HTML HOWEVER, the directive expansion is apparently not recursive, because the |
This is a bit hacky because it assumes that the only input is the trip object. There are likely ways to get around it through clever dom manipulation by using the `link` method but I am not going to overengineer here. This is basically the `ng-if` idea outlined in: e-mission/e-mission-docs#674 (comment)
…ned trips The changes were actually fairly minor - add new `get_confirmed_timeline` and `get_confirmed_timeline_from_dt` which reads cleaned places but confirmed trips - fortunately, the timeline linkage code focuses on trips, so it works with confirmed trips instead of cleaned trips - change the code which looks up the sections and stops to use the cleaned_trip id instead of the trip id if it is a confirmed trip TODO: change this if/when we have confirmed sections. We now get back a geojson with the inferred label fields. This completes the server changes outlined in e-mission/e-mission-docs#674 (comment)
|
One more pending change is to only retrieve non-processed user inputs from the server for the diary code as well. Right now, we are retrieving all user inputs. This list can grow without bound, which is generally a Bad Idea for system design. with this change, we will only read inputs from the timeline run point to now, which is bounded, and should be short depending on how frequently the timeline runs. |
Simplify the retrieval code by:
- pulling out duplicated code from the original call and the local fallback
into a separate function (`readTripsAndUnprocessedInputs`)
- pulling out the processing of the trips and the manual input handling for
future unification with the labeling code
- in the common case, only read inputs *after* the pipeline is complete.
Due to e-mission/e-mission-server#837, the geojson
trips should also now have any prior user inputs included in the trip.
This plugs a huge performance issue for longitudinal data collection, because
otherwise, we were reading the entire list of user inputs from the
beginning of the install, which grows unboundedly.
with this change, we will only read inputs from the timeline run point to
now, which is bounded, and should be short depending on how frequently the
timeline runs.
This addresses:
e-mission/e-mission-docs#674 (comment)
|
Last change done! Will close this after merging. |
|
Closing this for now. Will open other issues if we encounter bugs. |
@asiripanich @atton16 I have now finished the force sync hack (e-mission/e-mission-phone#797) and will start work on the user label handling. If I have to go back and look at this anyway, I will probably refactor some of the current code around
mode_confirm,purpose_confirmetc as well, including on the server.@PatGendre I will provide a migration script in case you are using the existing
manual/XXXlabels in your database.The text was updated successfully, but these errors were encountered: