search

sfu-natlang / lensingwikipedia

Lensing Wikipedia is an interface to visually browse through human history as represented in Wikipedia. This the source code that runs the website:
http://lensingwikipedia.cs.sfu.ca
Other
11 stars 4 forks source link

Redoing frontend queries API #190

Open theq629 opened 8 years ago

theq629 commented 8 years ago

I made the current frontend query system (queries.js) based on the original LensingWikipedia but before we had a clear picture of how many things should work. In retrospect I think it's much more complex than it needs to be. This is my recent thoughts on how to redo it. None of this relates to the protocol for sending queries to the backend, which I think is still fine.

Redundancy with selections system

The new selections system (#183) encapsulates most of the places where directly using constraints (Queries.Constraint) was needed. In general I'm finding the selections interface much more convenient to use. It directly represents the state of the view in ways the constraints don't (for example the map merges all selections of points into a single constraint), while still making it easy to synchronize the selection state with constraints.

Therefore in conjunction with the specific changes below it is likely possible to simplify the constraints system in favour of using the selections system. Of course we do still want to be present the constraint list in the UI, and there may be some unusual uses of constraints to consider (eg I believe there is one in compare.js that can't be wrapped by selections).

Constraint watchers

I think the choice to have watcher callbacks for individual constraints (adding and removing) was a bad idea. It leads to complex code and requires many separate callbacks instances. I think it would be better to have per-query watchers that are triggered when constraints are added or removed; that is, instead of doing someConstraint.onChange(...), you would have someQuery.on('change', ...) and have the callback filter for the constraints it's interested in. Additionally, I think this helps with the state saving situation (below).

With use of the selections system there are relatively few uses for watchers on constraints anyway. It's basically only for the UI's list of constraints, and so that the selection-query synchronizing code knows when a constraint was deleted through the UI list.

Combining queries

The original query system was based around the possibility of combining queries in various ways to get new queries. This leads to some complexity in queries having to be able to synchronize themselves with other queries to work out which constraints they actually contain at any moment. However, the only combination we've actually needed to implement and use is set-minus, since most views just want to get the global query result and the result without the view's own constraints. If we can agree on what range of operations we might need in the future we could probably simplify things.

Additionally, if we use the selections interface more then some of the complexity in combined queries can likely be removed. Especially there may be no need to for watcher callbacks which can detect when a constraint in a combined query is added or removed.

Saving state

In #125 @vlad003 noted that the current watcher system was an issue for saving state. The change to constraint watchers would presumably make this easier. However, I think we could probably save state in terms of selections rather than constraints. On restoring a saved selection it would just trigger the same watcher callbacks that would be triggered if the user had made those selections, and the code that synchronizes the selection with constraints would run normally. In contrast if we save constraints directly then it's hard for the views to reconstruct their state, eg the map view would need to parse its single constraint to work out which points were selected. Since constraints are independent I don't think we need to worry about order of selections. The same selections interface could also be used for any UI things that need to be saved (eg the choice of facet in the storyline view).

Practicality

Although this is a fairly large and complex part of the code to redo, it's not necessarily too much trouble. The selections interface already standardizes most of the interaction with constraints. Most views also use queries in a very standard way. Therefore it's fairly easy to work out which parts of the query API we actually need.

anoopsarkar commented 8 years ago

I like this. How do we implement this on the cheap? Most aspects are worked out in this proposal with the exception of Combining Queries which leaves some details unspecified. But awareness of the current source is still required so dropping in a novice to do this is probably a bad idea.

theq629 commented 8 years ago

Yes, I think we definitely someone familiar with the current code for this. Once we have a full plan I'm not sure the changes will be too hard. We'll have to be careful not to break anything, but I think we'll largely be simplifying code.

I think the other part that still needs to be specified is the details of how the new API should work. The planning steps are probably:

theq629 commented 8 years ago

I've been thinking more about how to do this. I haven't done the complete review of code but did go through the parts most likely to be effected.

Splitting up the Query class

A main source of complexity is the change watchers which get triggered when constraints are added or removed to/from queries. Besides the design issues with per-constraint watchers noted above, this also means that for setminus queries (or other potential combined queries) we have to watch and pass up change events, eg a constraint can be removed from a setminus query because it was added to only of the setminus query's parent queries.

However, in fact we seem to use change queries for exactly three things:

  1. some places clear their view or set a loading indicator when a query changes (because there will be a new result from the backend shortly)
  2. the constraints list box UI needs to know when constraints are removed
  3. selected data elements corresponding to constraints need to become unselected when a constraint is removed

(1) can be replaced by a new event triggered when a frontend query sends off to the backend for data. (2) and (3) only need the global query's constraints.

Therefore we can split the Query class up into separate classes for the global query, a regular query (custom set of constraints), and setminus query which manage constraints differently.

Further, the we can move handling of results (data from backend) out of the Query class(es) and into the ResultWatcher class.

Note that I think it is necessary to keep a special class for the global query even though it's basically a union of all regular queries, because that's how our interface model works -- especially, the constraints list UI needs to be able to remove any constraint regardless of what view it's from, so it needs a consistent remove operation.

Complete proposal

The Constraint class becomes simply an immutable wrapper for a JSON fragment (currently it is mutable and accepts null state) plus a text title. It has no watchers itself.

The Query class is split up into the following:

ResultWatcher is replaced with a View class. A View takes a Query and a JSON fragment view specification, and supports result (data from backend arrives) events and invalidation (requesting new data from backend) events. The JSON fragment probably needs to be mutable.

I believe this addresses the major issues:

Possible renaming

We might also want to take the chance to rename things to correspond better to the terms the backend and JSON interface use. The Query classes should become something like ConstraintSet, and View should become Query.

theq629 commented 8 years ago

I just did the full code review and can't find any issues, so does the above plan sound ok?

theq629 commented 7 years ago

Done in the queriesrefactoring branch.

API

Constraint: single constraint, immutable Connection: manages whole connection to the backend ConstraintSets.ConstraintSet: basic set of Constraints, with change watchers ConstraintSet.SetMinus: setminus set, no change watchers Queries.Query: basic query type, basically a constraint set - view specification pair, emits events on data changes Queries.PaginatedQuery: extends Query to support pagination

UI Changes

  1. Constraints in the constraint list box may appear to jump around when a view selection changes. This is because internally constraints are immutable now.
  2. Loading indicators pop up over their views. This is because with the new interface to query data the loading state tends to trigger sooner so otherwise you spend a lot of time with no data on the screen.