Skip to content

support getting bulk elements for tables based on spec #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shakeh merged 3 commits into from
Sep 13, 2018
Merged

support getting bulk elements for tables based on spec #66

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
206fae7
support getting bulk elements for tables based on spec
Aug 22, 2018
fbc7e1a
revert to using array until the end
Aug 28, 2018
4c6221b
Merge branch 'develop' into feature/VE-1041
Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions src/services/ViewService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ function ViewService($q, $http, $rootScope, URLService, ElementService, UtilsSer
.then(function(view) {
var toGet = [];
var results = [];
var toGetSet;
if (view._displayedElementIds) {
var displayed = view._displayedElementIds;
if (!angular.isArray(displayed)) {
Expand All @@ -171,22 +170,29 @@ function ViewService($q, $http, $rootScope, URLService, ElementService, UtilsSer
}
}
}
if (view.specification) {
if (view.specification && view.specification.operand) {
var specContents = view.specification.operand;
for (var j = 0; j < specContents.length; j++) {
if (specContents[j] && specContents[j].instanceId) {
toGet.push(specContents[j].instanceId);
}
}
}
toGetSet = new Set(toGet);
if (isTable(view) && view.specification && view.specification.value) {
try {
var tableJson = JSON.parse(view.specification.value);
if (tableJson.body) {
collectTableSources(toGet, tableJson.body);
}
} catch (e) {
}
Copy link
Contributor

@borromeotlhs borromeotlhs Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should throw something, no? Otherwise, you're making a silent error with this change that will be difficult to find when it fails (as it must, otherwise we wouldn't be "trying", right?).

Copy link
Collaborator Author

@dlamoris dlamoris Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case we just want it to continue as a failed json parse isn't necessarily an error and is not critical

Copy link
Contributor

@borromeotlhs borromeotlhs Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I can't argue with that logic. Though, I disagree with it whenever I see it, as it feels like we could stuff a json response of "failed to parse", without specifying actual error handling behavior and at no real cost. . .

I do like abdicating error handling, but I dislike wasting an opportunity to help the "me" that has to debug later. ;)

}
$http.get(URLService.getViewElementIdsURL(reqOb))
.then(function(response) {
var data = response.data.elementIds;
for (var i = 0; i < data.length; i++) {
toGetSet.add(data[i]);
}
toGet = toGet.concat(data);
}).finally(function() {
var toGetSet = new Set(toGet);
Copy link
Contributor

@borromeotlhs borromeotlhs Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should reference toGet as you only assumed success in the 'then' clause. This will mean that, regardless of success or failure of the promise to return, your finally is NOW dependent on success. Based on the content of this finally clause, the code within it should probably be moved up, and the original $http.get call should be chained with the 'getElements' promise being made in the finally clause. Of course, if the code below doesn't change, referencing a variable that may not exist on failure of the original promise is a moot point. Or, I could be totally misunderstanding: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally

Copy link
Collaborator Author

@dlamoris dlamoris Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toGet is declared in the higher context, if the get succeeds it just adds to it, otherwise we proceed with what we already had

Copy link
Contributor

@borromeotlhs borromeotlhs Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

reqOb.elementIds = Array.from(toGetSet);
ElementService.getElements(reqOb, weight, update)
.then(function(data) {
Expand All @@ -204,6 +210,24 @@ function ViewService($q, $http, $rootScope, URLService, ElementService, UtilsSer
return deferred.promise;
};

var collectTableSources = function(sources, body) {
var i, j, k;
for (i = 0; i < body.length; i++) {
var row = body[i];
for (j = 0; j < row.length; j++) {
var cell = row[j];
for (k = 0; k < cell.content.length; k++) {
Copy link
Contributor

@borromeotlhs borromeotlhs Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like underscore.js or even jQuery should have an iterable class that would allow for less error prone iteration of the table to non-recursively and with constant memory do what's being done here, right? This looks like a loop-unrolling optimization, honestly.

Copy link
Collaborator Author

@dlamoris dlamoris Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the simple for loop is usually fastest compared to other library based iterations, and we deal with a lot of huge tables, unless we need a function scope for each element, we prefer for loops for dealing with tables

Copy link
Contributor

@borromeotlhs borromeotlhs Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true enough.

var thing = cell.content[k];
if (thing.type === 'Table' && thing.body) {
collectTableSources(sources, thing.body);
} else if (thing.type === 'Paragraph' && thing.source) {
sources.push(thing.source);
}
}
}
}
};

/**
* @ngdoc method
* @name mms.ViewService#getDocumentViews
Expand Down