Ongoing PR to track the progress of modularizing and modernizing user input handling #799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Testing done: - Load data for 2016-08-04 - Visit the day in the UI - verify that the `<multilabel>` element is rendered ``` <multilabel inputs="['mode', 'purpose']" class="ng-binding">inputs are ["mode","purpose"]</multilabel> ```
…eholder
Move out the HTML for the button bar into "multi-label-ui.html"
Move out the controller code to manipulate the popup and save the resulting data
Add a new input for the list of valid labels since the code currently resets the list
```
$scope.inputParams = {}
```
so we want to do that at the screen level, and not the individual directive level.
Next steps:
- Figure out how to properly refactor the remaining references to
ConfirmHelper.INPUTS, notably related to:
- `$scope.$on('$ionicView.loaded', function() {` and
- `$scope.populateInputFromTimeline(`
|
Everything works now (including adding other entries to the lists), but the code could stand to be prettier.
|
Merged
+ 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!
- Remove the unused link function - Simplify logs by removing several of them and reducing the intensity of others (by not logging the entire object)
so it takes up the entire space in the card like it used to before these changes started
it is fairly complex, so better to pull it out instead of having it be inline with the directive definition. Now that we know how to include it, we can even move it out to a separate file if it gets really complicated.
|
Current screenshot showing proper spacing after adding |
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!
|
Example of auto-filled labeling. Note that the promise to fill in is launched early when the screen is still blank. Note also that everything except the labels is filled in first, and then the labels are filled in when the timeout finishes with no user interaction.
Patch to slow down option resolution also attached here... |
Remove all vestiges of the ConfirmHelper code from `list.js`
```
$ grep ConfirmHelper www/js/diary/list.js | wc -l
0
```
Also remove the list of supported inputs from the directive to avoid
duplication with the list in the `ConfirmHelper`. Later, we should clean up
`ConfirmHelper` to read that from the config instead of putting it into code.
Or if we choose to pass it into the directive, we have to figure out how to
pass it in to the factory at creation time.
There is no point in configuring it in two places.
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
|
Current status: both label view and diary use the same directive, and it works for green (confirmed) labels
|
… screen Detailed investigation into the linkage is at: e-mission/e-mission-docs#674 (comment) to e-mission/e-mission-docs#674 (comment) Also changed the formatting of the diary screen a bit to clear some space for the verify check button. Note that we needed to reduce the scope of the `toDetail` call in that case, since it will result in both confirming and going to the detail screen. Testing done: Clicked on the "VC" button in both the diary and the infinite list screens. Got the click and found the corresponding multilabel in both cases. ``` element is S.fn.init [verifycheck.col-10.diarycheckmark-container] 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: 2455, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …} ```
|
Current status: both diary and label screens use the same "verify check" button. When the button is clicked, we can access the scope of the associated
|
- Pass in the tagName of the label button to verifycheck - Look up element with the tagName dynamically (similar to eb1fd97) - Actually invoke a method on its scope + move the verifyTrip code from `infinite_scroll_list.js` to the multi-label code so that it can be invoked from the scope. + move the verifiability initialization from infinite_scroll_list to the verify directive since it is basically initialization code + Move the verify check HTML code from the `infinite_scroll_list` HTML code to a separate template HTML so that it actually shows a check mark instead of "VC"
|
Current state: Diary and labels both have check marks instead of VC
And clicking on the check mark calls the scope of the linked object. |
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.
|
Current state: code has been filtered out of both views, and the popups work properly
|
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.
…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
- remove the duplicate implementations of the code that populates entries - call `populateInputsAndInferences` directly - fill in the fields needed for it to work ahead of time Testing done: - Loaded diary, no errors - Loaded label, no errors
Since `getUserInputForTrip` is now called only from `populateManualInput` we can adapt it to take the trip inputs directly, and we don't have to massage the input to meet expecations The one minor ordering change to accomplish this was to compute the basic classes before matching the inputs so that we can use the `isDraft` variable directly. We don't (currently) show draft trips on the label screen, so it will be undefined there. And undefined != true, so it will only apply for diary trips.
|
Current state: both diary and list view load correctly, and values can be selected correctly from the drop down. |
- Remove the unused dependency on `$q` (we use built-in promises instead) - Remove the page reload on background sync which was commented out a long time ago; we may also be removing/refactoring the background sync anyway - Remove `explainDraft`, `makeCurrent` from the label screen since we don't show draft trips there - Remove `prevDay`, `nextDay` from the label screen since we don't use day-based navigation - Remove the current trip code from the label screen since we don't have a way to launch it - Remove the code around groups which seems to have been added in (e-mission@1e95720), has no documentation, uses `good1`, `good2` and `good3` as the groups - Add client stats to the diary code, similar to the label view - Remove unused `getTimeSplit` from both screens - Remove the code to refresh the maps in the diary view since we have removed that button - Remove unused `isNotEmpty` - Ensure that we don't consider the position to refresh since we don't pull to refresh any more
Remove all the references from the diary helper to `$scope` variables. These were added back when we displayed common trips (e-mission@c393e3d) in order to avoid duplicating the common trip information between the list view and the common trips code. We then used them directly in the UI to format data for display. However, as part of e-mission@30561b9 we changed the template to populate values from trip variables instead of `$scope` functions. So this reassignment is no longer required. Let's remove them and replace them with direct calls to the diary helper.
In e-mission@a96194d, we removed the references to `DiaryHelper` methods in `$scope`. In this follow-on, we change `populateCommonInfo` to invoke calls directly from the DiaryHelper instead of the `$scope`. Testing done: - Does not generate errors while populating the list view
- survey/external: externally launched surveys - survey/multilabel: multiple labels - survey/enketo: (in the future) for eneketosurvey - survey/hexmap: (in the future) for itinerum surveys + Fixed all paths in index.html + Changed all the module names + Changed all module imports in the files that used them
- We move out the templates from `tripconfirm` to `survey/multilabel` and change all the template loading code as well. - We also create a top layer `survey.js` which imports the relevant modules from the survey implementations. This allows the code which uses the values e.g. `list.js`, `infinite_scroll_list.js` and loads the survey dynamically, to include the survey module directly and not have to change it to switch between implementations.
|
At this point, we have moved the code around so the surveyed part is all together. We have also created a There are no instances of All instances of multilabel are either within the module, or related to posttrip (out of scope) or as elements in the HTML. |
- 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
- Remove the old one-click-button!! - Change the module name - Remove unnecessary dependencies Current hardcoded status is: ``` grep -r multilabel www/ | egrep -v "survey/multilabel|posttrip" www//js/survey/survey.js: "emission.survey.multilabel.buttons", www//js/survey/survey.js: "emission.survey.multilabel.infscrollfilters", www//js/survey/survey.js: elementTag: "multilabel" www//templates/diary/list.html: <multilabel class="col" trip="tripgj"></multilabel> www//templates/diary/infinite_scroll_list.html: <multilabel class="col" trip="trip"></multilabel> ```
- Remove logging that happens on every directive init to improve performance
- Fix the template URL for the verify button
- Fix the case on the `linkedTag`
- Remove unused scope variables from the verify button, both in the
definition and in the usage
Testing done:
Clicked the verify button; it worked
$element is S.fn.init [verifycheck.row] linkedtag is multilabel
index.html:145 parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 child linkedlabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
index.html:145 linkedlabel scope is Scope {$id: 194, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: Scope, …}
index.html:145 About to verify trip 1470341031.235 -> 1470342912 with current visibilityalready-verified
$element is S.fn.init [verifycheck.col-10.diarycheckmark-container.center-vert.center-horiz] linkedtag is multilabel
index.html:145 parent row is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 row Element is S.fn.init [ion-item#diary-item.list-item.item, prevObject: S.fn.init(1)]
index.html:145 child linkedlabel is S.fn.init [multilabel.col, prevObject: S.fn.init(1)]
index.html:145 linkedlabel scope is Scope {$id: 2258, $$childTail: Scope, $$childHead: Scope, $$prevSibling: Scope, $$nextSibling: null, …}
index.html:145 About to verify trip 1470882349.3511817 -> 1470883723.49 with current visibilityalready-verified
Current hardcoding status:Current display:
Current verify status:Diary tabLabel tab |
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)
|
At this point, we create the multilabel element as a child of the linked survey in both the diary and the list view |
Copy over the existing user input and user inferred labels from the trip properties to the top level. This allows the existing inference code to work without any changes. Yay!
Since the variable is no longer passed in. And it can't be passed in because the `verifyTrip` is actually invoked from outside this scope by the `verifycheck` directive. We couldn't test this before because all the trips were already with green labels. Tested with a yellow label and it worked.
|
At this point, we can see the inferred labels in the diary screen and even verify them with the single click!
|
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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the phone component for e-mission/e-mission-docs#674