Closed shankari closed 3 years ago
I have been thinking for a while that, instead of having separate manual/mode_confirm
, manual/purpose_confirm
, manual/replaced_mode_confirm
...
we may want to have a data structure with key manual/user_input
and a more complex structure:
user_input: {
mode:
purpose:
replaced_mode:
}
I can see three possible advantages with this change:
user_input: {
name:
address:
...
}
the user input code can be significantly simplified since it no longer needs to work with multiple inputs. The confirmed_trip code already takes existing manual labels and converts them into this structure. After the change, we could just copy over the entire structure instead of specifying the user input labels at
https://github.com/e-mission/e-mission-server/blob/9ce6aacf328866c74ca5cdc754c0c9a03591aa3d/conf/analysis/debug.conf.json.sample#L11
"userinput.keylist": ["manual/mode_confirm", "manual/purpose_confirm"]
and change it every time
deployers who want to add new semantic data would no longer have to add a new data type, a non-trivial task. Instead, they could just store the user input in a different format, and the existing pipeline, including the autolabeling, would Just Work.
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.
I will provide a migration script in case you are using the existing manual/XXX labels in your database.
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 userInput.data.label
to display survey response shown in the pic below.
The fact that the proposed user_input
allows arbitrary number of fields make it flexible support other input type. This is a great plus.
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 data
inside manual/survey_response
:
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.
@PatGendre
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.
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
{
_id:
uuid:
metadata:
data:
}
If the timestamp
represents the time the the object was written, that is also captured in metadata.write_ts
.
Having standard fields for all data structures (uuid, write_ts) but allowing the data to be more flexible gives us the best of both worlds.
The rest of the fields make sense. I would suggest that you should convert the xmlResponse
to JSON before storing since it can then be the basis of mongodb queries.
@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
{ _id: uuid: metadata: data: }
If the
timestamp
represents the time the the object was written, that is also captured inmetadata.write_ts
. Having standard fields for all data structures (uuid, write_ts) but allowing the data to be more flexible gives us the best of both worlds.The rest of the fields make sense. I would suggest that you should convert the
xmlResponse
to JSON before storing since it can then be the basis of mongodb queries.
I could convert xmlResponse
to json but I also need it for internal enketo library to work. Best to add jsonResponse
which is the json-converted version of xmlResponse
.
Edit:
My concern on uuid
and timestamp
is that the metadata
and the provided user_id
field is not ready until it is pushed. That is why I also embed it in data
object.
My concern on uuid and timestamp is that the metadata and the provided user_id field is not ready until it is pushed. That is why I also embed it in data object.
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 userInput.data
to retrieve values; it is because there is also a userInput.metadata
.
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.
return element.metadata.write_ts == value.metadata.write_ts;
My concern on uuid and timestamp is that the metadata and the provided user_id field is not ready until it is pushed. That is why I also embed it in data object.
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
userInput.data
to retrieve values; it is because there is also auserInput.metadata
.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.
return element.metadata.write_ts == value.metadata.write_ts;
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 list.js
.
These are:
$scope.populateInputFromTimeline
, which is called when post-processing the loaded trips for the day to match the inputs$scope.$on('$ionicView.loaded', function() {
which fills in the default input parameters and mapsThe obvious design decision for the first choice is to populate the value in the directive. This has a couple of nice advantages:
However $scope.populateInputFromTimeline
requires us to have both the current trip and the next trip. This is because it calls DiaryHelper.getUserInputForTrip
, which checks that the end of the matched user input is before the start of the next trip. https://github.com/e-mission/e-mission-phone/pull/738/commits/a70569ec09b3ffd924aa0eb2bd8f4c2b80148b7e
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 tripgj in data.currDayTripWrappers
. So we can:
currDayTripWrappers
be a linked listOptions (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.
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 "after loading in directive
".
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
after loading, inputParams = {}
So the initial load doesn't initialize anything, but we do have entries before we start matching inputs
index.html:145 after loading, inputParams = {}
index.html:145 While populating inputs, inputParams MODE: options: (9) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}] PURPOSE: {options: Array(15), text2entry: {…}, value2entry: {…}}
index.html:145 While populating inputs, inputParams MODE: options: (9) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}]text2entry: value2entry: PURPOSE: {options: Array(15), text2entry: {…}, value2entry: {…}}
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
index.html:145 While populating inputs, inputParams Object
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.
index.html:145 TypeError: Cannot read property 'value2entry' of undefined
at Scope.$scope.populateInputFromTimeline (multi-label-ui.js:29)
at multi-label-ui.js:48
at Array.forEach (<anonymous>)
at Scope.$scope.fillUserInputs (multi-label-ui.js:47)
at multi-label-ui.js:58
at Scope.$digest (ionic.bundle.js:30239)
at render (ionic.bundle.js:62720)
at forceRerender (ionic.bundle.js:62618)
at RepeatController.refreshLayout (ionic.bundle.js:62589)
at refreshDimensions (ionic.bundle.js:62344)
ah, we do load the data, we just do so asynchronously. ConfirmHelper.getOptionsAndMaps
returns a promise.
Next step: can we get the event in the directive so we can move the code there?
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 (ConfirmHelper
)?
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
diff --git a/www/js/tripconfirm/trip-confirm-services.js b/www/js/tripconfirm/trip-confirm-services.js
index 5094f057..24be3163 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 = {
@@ -82,12 +82,12 @@ angular.module('emission.tripconfirm.services', ['ionic', 'emission
.i18n.utils',
ch.getOptions = function(inputType) {
if (!angular.isDefined(ch.inputDetails[inputType].options)) {
var lang = $translate.use();
- return loadAndPopulateOptions()
+ return $timeout(() => loadAndPopulateOptions()
.then(function () {
return ch.inputDetails[inputType].options;
- });
+ }), 60000);
} else {
- return Promise.resolve(ch.inputDetails[inputType].options);
+ return $timeout(() => Promise.resolve(ch.inputDetails[inputType].options),
10000);
}
}
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.
ionic view loaded event invoked
in loaded callback, inputParams for MODE = undefined
in loaded callback, inputParams for PURPOSE = undefined
registering loaded callback in directive
registering loaded callback in directive
TypeError: Cannot read property 'value2entry' of undefined
TypeError: Cannot read property 'value2entry' of undefined
GET http://localhost/_app_file_/data/user/0/edu.berkeley.eecs.emission.devapp/files/phonegapdevapp/www/json/trip_confirm_options.json 404 (OK)
In loaded callback, processing {options: Array(9), text2entry: {…}, value2entry: {…}}
In loaded callback, processing {options: Array(14), text2entry: {…}, value2entry: {…}}
Waiting for trip confirm options load | Successful load, then manual refresh |
---|---|
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. According to https://stackoverflow.com/questions/21496331/are-angularjs-services-singleton all angular services (and factories and providers) are essentially singletons, so this should be simple. We just have to make sure that we return an object instead of a function that returns an object.
With that, the ConfirmHelper
interface becomes super simple - it just exports the inputParams. Everything else about reading the data is internal to it. But it will have to return it as a promise; will that still work?
After adding some logs:
let c1 = ConfirmHelper;
let c2 = ConfirmHelper;
console.log("Is this a singleton?" + (c1 == c2), c1, c2);
let p1 = ConfirmHelper.getOptionsAndMaps("MODE");
let p2 = ConfirmHelper.getOptionsAndMaps("MODE");
console.log("Are the promises identical?" + (p1 == p2), p1, p2);
Promise.all([p1, p2]).then((r1, r2) => {
console.log("Are the results identical?" + (r1 == r2), r1, r2);
});
We get the following logs:
Is this a singleton?true
{INPUTS: Array(2), inputDetails: {…}, getOptionsAndMaps: ƒ, getOptions: ƒ, checkOtherOption: ƒ, …}
{INPUTS: Array(2), inputDetails: {…}, getOptionsAndMaps: ƒ, getOptions: ƒ, checkOtherOption: ƒ, …}
Are the promises identical?false Promise {<pending>} Promise {<pending>}
index.html:145
Are the results identical?false
(2) [{…}, {…}]
options: (9) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}]
text2entry: {Walk: {…}, Bike: {…}, Drove Alone: {…}, Shared Ride: {…}, Taxi/Uber/Lyft: {…}, …}
value2entry: {walk: {…}, bike: {…}, drove_alone: {…}, shared_ride: {…}, taxi: {…}, …}
options: (9) [{…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}, {…}]
text2entry: {Walk: {…}, Bike: {…}, Drove Alone: {…}, Shared Ride: {…}, Taxi/Uber/Lyft: {…}, …}
value2entry: {walk: {…}, bike: {…}, drove_alone: {…}, shared_ride: {…}, taxi: {…}, …}
Since the service is a singleton, we should be able to have a single inputParams
object it in. We should be able to populate it directly as the object is created instead of having the calling location invoke a function. However, it needs to be populated with a promise, and we need to know when the promise is complete.
We need to know that the data is present before we call populateInputFromTimeline
At this point (with https://github.com/e-mission/e-mission-phone/pull/799/commits/c9621c30506f54de8b4a33d4d9db40a0310ac072), we have removed all vestiges of the ConfirmHelper
from list.js
grep ConfirmHelper www/js/diary/list.js | wc -l
0
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:
Change | Resolution |
---|---|
padding style for the enclosing div |
Combine all of them |
style v/s class for the enclosing div |
Retain the class; it was designed for use with the walkthrough and may be used for it again |
renaming tripgj v/s trip | change name in the directive to trip |
Use of finalInference |
Incorporate into the directive |
wrt the input-confirm-row
, it was added in https://github.com/e-mission/e-mission-phone/commit/8113fccc52754fb285d3f7321737b9d76ba7c050 to make it easier to walk through the label screen. However, in
https://github.com/e-mission/e-mission-phone/commit/62da14ca82d8f0eebb334acb6826b18b868d0ccd I changed the walkthrough to use '.diary-entry'
instead of input-confirm-row
because of https://github.com/e-mission/e-mission-docs/issues/669
So it is not currently used anywhere.
$ grep -r input-confirm-row www/
Binary file www//templates/diary/.infinite_scroll_list.html.swo matches
www//templates/diary/infinite_scroll_list.html: <div class="row input-confirm-row">
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 div
was changed in
https://github.com/e-mission/e-mission-phone/commit/9cf8258334eee62fc388baade1f104847fd67820
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 (https://github.com/e-mission/e-mission-phone/commit/85a56ba4c11cda1bb9861830b4d4d98a5d5fa85f)
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.
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? Pro: It is part of the labeling Cons:
Options:
requires
controllers, although I am not sure if it works for sibling controllers.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.
@GabrielKS in https://github.com/e-mission/e-mission-phone/commit/9cf8258334eee62fc388baade1f104847fd67820, change from
<div class="row" style="padding-left: 5px;padding-right: 5px;margin-top: 0px">
<div ng-repeat="input in userInputDetails" class={{input.width}} style="text-align: center;font-size: 14px;font-weight: 600;" ng-attr-id="{{ 'userinputlabel' + input.name" translate>
{{input.labeltext}}
</div>
</div>
to
<div class="row" style="margin-top: 0px">
<div ng-repeat="input in userInputDetails" class={{input.width}} style="text-align: center;font-size: 14px;font-weight: 600;" ng-attr-id="{{ 'userinputlabel' + input.name" translate>
{{input.labeltext}}
As I continued working on this overnight, I picked the revised spacing, under the argument that you must have had a reason for it. And it seems to be working fine so far 😄
https://github.com/e-mission/e-mission-phone/pull/799#issuecomment-929905002
Continuing with https://github.com/e-mission/e-mission-docs/issues/674#issuecomment-930224805 I don't see a way to require sibling controllers, but the documentation around this is unclear. Let's just try it and see what happens.
.directive('verifycheck', function() {
return {
scope: {
},
require: '^^<various>',
link: function(scope, element, attrs, multiLabelCtrl) {
console.log("Initialized link function with required controller ", multiLabelCtrl);
},
template: 'This is the verify check'
};
})
MultiLabelCtrl
doesn't workError: [$compile:ctreq] Controller 'MultiLabelCtrl', required by directive 'verifycheck', can't be found!
DiaryListCtrl
doesn't workError: [$compile:ctreq] Controller 'DiaryListCtrl', required by directive 'verifycheck', can't be found!
InfiniteDiaryListCtrl
doesn't workcontroller InfiniteDiaryListCtrl called
Error: [$compile:ctreq] Controller 'InfiniteDiaryListCtrl', required by directive 'verifycheck', can't be found!
There is probably a good answer for this, but given that it is not even clear that this will work for sibling directives, I think we should abandon this approach.
Another option is to have a shared data structure. If we pass in the same trip object to both the labels and the verify button, then the checkmark button can modify the trip directly and the $watch
mechanism should result in everything getting updated.
The main issue with this approach is that we will potentially have to duplicate the save
code. We can work around that by pulling the save functionality out into a shared service.
Note that the current implementation of save between the diary and the scroll list is already slightly different, so we will have to add additional config options at some point...
To finish the controller option:
controller: 'InfiniteDiaryListCtrl',
). This "worked", but resulted in two instances of the controller.controller InfiniteDiaryListCtrl called
controller InfiniteDiaryListCtrl called
Trying to manually find the sibling controller using https://exceptionshub.com/angular-get-controller-from-element.html
Added the following link function:
link: function(scope, element, attrs, controllers) {
console.log("element is ", element);
console.log("verifycheck element is", angular.element("verifycheck"));
console.log("multilabel element is ", angular.element("multilabel"));
},
And moved the <verifycheck/>
tag into the ion item.
<verifycheck/>
<div class="row" style="padding-left: 5px;padding-right: 5px;">
<multilabel class="col" trip="tripgj" unified-confirms-results="data.unifiedConfirmsResults"></multilabel>
</div>
It doesn't find the item, probably because the DOM is initialized top to bottom.
element is S.fn.init [verifycheck]
verifycheck element is S.fn.init(19) [verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, prevObject: S.fn.init(1)]
multilabel element is S.fn.init [prevObject: S.fn.init(1)]
Moving it below
<div class="row" style="padding-left: 5px;padding-right: 5px;">
<multilabel class="col" trip="tripgj" unified-confirms-results="data.unifiedConfirmsResults"></multilabel>
</div>
<verifycheck/>
does work,
element is S.fn.init [verifycheck]
verifycheck element is S.fn.init(20) [verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, verifycheck, prevObject: S.fn.init(1)]
multilabel element is S.fn.init(20) [multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, multilabel.col, prevObject: S.fn.init(1)]
but:
OK!! I have figured out how to deal with the multiple multilabel tags through judicious use of jquery. You find the parent item, then find the appropriate child directive and wrap it in an angular element (https://docs.angularjs.org/api/ng/function/angular.element#!). This then allows us to access the directive scope and see the store method that we want to access.
console.log("element is ", element);
console.log("parent row is", element.parents("ion-item"));
let rowElement = element.parents("ion-item")
console.log("row Element is", rowElement);
let multilabel = rowElement.find("multilabel");
console.log("child multilabel is", multilabel);
console.log("angular wrapper on child multilabel is", angular.element(multilabel));
let multilabelScope = angular.element(multilabel).isolateScope();
console.log("multilabel ctrl is", multilabelScope);
console.log("Checking scope for first element", angular.element($("multilabel")[0]).isolateScope());
let allml = $("multilabel");
for (let i = 0; i < allml.length; i++) {
console.log("Checking scope for element at "+i, angular.element(allml[i]).isolateScope());
}
$timeout(() => {
console.log("after 5 secs, angular wrapper on previously retrieved child multilabel is", angular.element(multilabel));
let multilabelScope = angular.element(multilabel).isolateScope();
console.log("after 5 secs, multilabel ctrl on previously retrieved child is", multilabelScope);
console.log("after 5 secs, angular wrapper on newly retrieved child multilabel is", angular.element(multilabel));
let multilabelNew = rowElement.find("multilabel");
let multilabelScopeNew = angular.element(multilabelNew).isolateScope();
console.log("after 5 secs, multilabel ctrl on newly retrieved child is", multilabelScopeNew);
let allml = $("multilabel");
for (let i = 0; i < allml.length; i++) {
console.log("after 5 secs, checking scope for element at "+i, angular.element(allml[i]).isolateScope());
}
}, 5000);
works
after 5 secs, multilabel ctrl on newly retrieved child is Scope {$id: 194, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
However, note that the scope is not available immediately. In fact, none of the directive scopes are available immediately. We need to wait for ~ 5 secs for everything to load, and in fact, we need to re-lookup the directive after 30 secs for the scope to show up.
element is S.fn.init [verifycheck]
parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
child multilabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
angular wrapper on child multilabel is S.fn.init [multilabel.col]
multilabel ctrl is undefined
Checking scope for first element undefined
Checking scope for element at 0 undefined
...
element is S.fn.init [verifycheck]
parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
child multilabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
angular wrapper on child multilabel is S.fn.init [multilabel.col]
multilabel ctrl is undefined
Checking scope for first element undefined
Checking scope for element at 0 undefined
Checking scope for element at 1 undefined
Checking scope for element at 2 undefined
Checking scope for element at 3 undefined
...
Invoked multilabel directive controller for labels MODE,PURPOSE
Finished initializing directive, userInputDetails = (2) [{…}, {…}]
Invoked multilabel directive controller for labels MODE,PURPOSE
Finished initializing directive, userInputDetails = (2) [{…}, {…}]
...
after 5 secs, angular wrapper on previously retrieved child multilabel is S.fn.init [multilabel.col]
after 5 secs, multilabel ctrl on previously retrieved child is undefined
after 5 secs, angular wrapper on newly retrieved child multilabel is S.fn.init [multilabel.col]
after 5 secs, multilabel ctrl on newly retrieved child is Scope {$id: 194, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
after 5 secs, checking scope for element at 0 Scope {$id: 194, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
after 5 secs, checking scope for element at 1 Scope {$id: 195, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
after 5 secs, checking scope for element at 2 Scope {$id: 196, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
after 5 secs, checking scope for element at 3 Scope {$id: 197, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
Moving the element lookup into the onClick
functionality should solve this timeout issue, as well as the ordering of the two directives. Let's try that now.
@GabrielKS in e-mission/e-mission-phone@9cf8258, change from
<div class="row" style="padding-left: 5px;padding-right: 5px;margin-top: 0px"> <div ng-repeat="input in userInputDetails" class={{input.width}} style="text-align: center;font-size: 14px;font-weight: 600;" ng-attr-id="{{ 'userinputlabel' + input.name" translate> {{input.labeltext}} </div> </div>
to
<div class="row" style="margin-top: 0px"> <div ng-repeat="input in userInputDetails" class={{input.width}} style="text-align: center;font-size: 14px;font-weight: 600;" ng-attr-id="{{ 'userinputlabel' + input.name" translate> {{input.labeltext}}
As I continued working on this overnight, I picked the revised spacing, under the argument that you must have had a reason for it. And it seems to be working fine so far 😄
Ah. I think I just moved it into style.css
(see the first change on the https://github.com/e-mission/e-mission-phone/commit/9cf8258334eee62fc388baade1f104847fd67820 page).
Bingo! That works!
element.on('click', function(event) {
console.log("element is ", element);
console.log("parent row is", element.parents("ion-item"));
let rowElement = element.parents("ion-item")
console.log("row Element is", rowElement);
let multilabel = rowElement.find("multilabel");
console.log("child multilabel is", multilabel);
let multilabelScope = angular.element(multilabel).isolateScope();
console.log("multilabel scope is", multilabelScope);
});
generates
element is S.fn.init [verifycheck]
parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
child multilabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
multilabel scope is Scope {$id: 194, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: Scope, …}
As expected, it also works if we move the button before the tag
element is S.fn.init [verifycheck.row]
parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
child multilabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
multilabel scope is Scope {$id: 134, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
However, the tag after the button is apparently not visible. Switching the tag format fixes it
<verifycheck class="row"/> |
<verifycheck class="row"></verifycheck> |
---|---|
Now that we use the multi-label directive in both tabs, we need to make sure that the implementation is the same. The infinite scroll list actually had slight variations to the labeling code (which is also why copy/paste is a bad idea). The two main differences to existing code are:
The resolution of these issues is:
data.properties.(start|end)_ts
to the tripupdateTripProperties
has three steps
inferFinalLabels
: needs to be run in the directive, since it not applicable (at least in the same way) to surveysupdateVerifiability
: needs to be run in the directive, since it checks whether everything is yellow etcupdateVisibilityAfterDelay
: needs to be run in the parent, since it calls recomputeDisplayTrips
, which, in turn, changes displayTrips
, which is the list of trips to be displayed in the UIOne final issue:
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.
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 $scope.data.displayTrips
. So the obvious fix is to pull out the filtering code in the directive into a service and invoke it from both the directive and the view controller.
In other words MultiLabelCtrl
calls ConfirmHelper.fillUserInputsObjects
; InfiniteDiaryListCtrl
also calls ConfirmHelper.fillUserInputsObjects
. So we can fill in all the userInput
fields. But this reintroduces the dependency between InfiniteDiaryListCtrl
and ConfirmHelper
instead of keeping the list control generic and independent of the labels.
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.
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 $watch
trigger is invoked as the user scrolls through the list. Note that the oldVal == newVal
.
the trip binding has changed from {display_end_time: "2:34 PM" display_start_time: "2:18 PM"} bo new value {display_end_time: "4:18 PM" display_start_time: "2:39 PM"}
the trip binding has changed from {data: {…}, start_place: {…}, style: ƒ, onEachFeature: ƒ, pointToLayer: ƒ, …} bo new value {data: {…}, start_place: {…}, style: ƒ, onEachFeature: ƒ, pointToLayer: ƒ, …}
...
This means that actually invoking code within the $watch
can seriously affect scrolling performance. So populating labels, which involves iterating over the manual labels and includes some complex matching, is not suitable for the $watch
. It is also redundant, since we only need to populate them once, not every time as we scroll.
And we need to initialize the service and precompute values for the label screen anyway.
So we remove the $watch
callback and call the service directly from the diary screen as well.
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 https://github.com/e-mission/e-mission-phone/pull/799#issuecomment-934522421, we should be done. Users can simply change from
<multilabel class="col" trip="trip"></multilabel>
to
<enketosurvey class="col" trip="trip"></enketosurvey>
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
surveyoptions.MULTILABEL = {
filter: "InfScrollFilters",
service: "MultiLabelService",
elementTag: "multilabel"
}
configuring the controller
$scope.surveyOpt = SurveyOptions.MULTILABEL;
and then changing the tag to
<{{surveyOpts.elementTag}} class="col" trip="trip"></{{surveyOpts.elementTag}}>
Unfortunately, that doesn't work since the element tag is treated as a string.
I tried some other dynamic matching options:
Didn't work because it wasn't expanded properly
Didn't work because of lack of matching, even though I changed the directive config to restrict: 'EAC',
WORKS!!
Double checking this once more because this is so crazy:
<div class="col multilabel" trip="tripgj" data-foo="{{surveyOpt.elementTag}}"></div>
<div class="col {{surveyOpt.elementTag}}" trip="tripgj" data-foo="{{surveyOpt.elementTag}}"></div>
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 ng-if
for the selection. That is outside the scope at this point, but I will probably return to it after the enketo survey is in place.
For the record, I also tried to create a new directive linkedsurvey
and pass in the element tag via a scope so it could be used in an inner template.
The hope was to use
<linkedsurvey element-tag="{{surveyOpt.elementTag}}" trip="tripgj" class="row"></linkedsurvey>
I was not able to get it to work:
templateURL
function doesn't take in the scope, so trying to load a dynamic template aka https://docs.angularjs.org/guide/directive#template-expanding-directive just ends up trying to load templates/survey/{{surveyOpt.elementTag}}-wrapper.html
compile
function also doesn't take the scope and will run into a similar issue. The link function does take the scope into account, but it also involves working with weird issues around replaceWith
which did not seem to actually replace anything for me. We also need to ensure that we copy the attributes over, otherwise, we will end with the tripgj
missing, and will not display anything.Finally, $compile just seems to be completely broken when I try to use it
link: function(scope, elem, attr) {
let template = "<div>{{elementTag}}</div>";
console.log("Compiled functon ", $compile(template));
let newElem = $compile(template)(scope);
console.log("after manual compile with ", scope.elementTag, template, newElem.contents());
elem.append(newElem);
}
Outputs
after manual compile with multilabel <div>{{elementTag}}</div>
S.fn.init(1)
0: text
data: "{{elementTag}}"
length: 14
nodeName: "#text"
nodeType: 3
nodeValue: "{{elementTag}}"
ownerDocument: document
parentElement: div.ng-binding
parentNode: div.ng-binding
previousElementSibling: null
previousSibling: null
textContent: "{{elementTag}}"
wholeText: "{{elementTag}}"
I don't know what $compile
is supposed to do if it doesn't fill in a template!!
Ah but hardcoded replaceWith
works.
.directive("linkedsurvey", function($compile) {
return {
scope: {
elementTag:"@",
},
link: function(scope, elem, attr) {
let newHTML = "<"+scope.elementTag+"></"+scope.elementTag+">";
elem.replaceWith(newHTML);
console.log("after manual compile with ", scope.elementTag, elem);
}
};
});
Doesn't seem to work in the logs
after manual compile with multilabel S.fn.init [linkedsurvey.col]
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
.directive("linkedsurvey", function($compile) {
return {
scope: {
elementTag:"@",
},
link: function(scope, elem, attr) {
console.log("before manual compile with ", scope.elementTag, elem, attr);
let newHTML = "<"+scope.elementTag+"></"+scope.elementTag+">";
let newElem = angular.element(newHTML);
for (let normalizedKey in attr.$attr) {
const domKey = attr.$attr[normalizedKey];
const domVal = attr[normalizedKey];
newElem.attr(domKey, domVal);
}
console.log("after manual compile with ", scope.elementTag, elem, newElem, attr);
elem.replaceWith(newElem);
}
};
This actually does generate the correct HTML
HOWEVER, the directive expansion is apparently not recursive, because the <multilabel>
directive is not expanded further. I give up.
One more pending change is to only retrieve non-processed user inputs from the server for the diary code as well. This should be a large performance improvement for long-term data collection.
Right now, we are retrieving all user inputs.
1120 var tq = $window.cordova.plugins.BEMUserCache.getAllTimeQuery();
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.
Last change done! Related phone PR: https://github.com/e-mission/e-mission-phone/pull/799 Related server PR: https://github.com/e-mission/e-mission-server/pull/837
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 (https://github.com/e-mission/e-mission-phone/pull/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_confirm
etc as well, including on the server.@PatGendre I will provide a migration script in case you are using the existing
manual/XXX
labels in your database.