Skip to content
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.

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

Enhancement: Allow user to load incomplete relation members #2284

Closed

Conversation

sogko
Copy link

@sogko sogko commented Jul 11, 2014

Description
This enhancement is to enable user to load incomplete relation members, shown as <not downloaded> in iD.ui.RawMemberEditor.
This is a simple change but enhances the user experience greatly, especially in managing relation entities.

By simply calling connection.loadEntity(relationId), we can fill in the missing related entities (ways / nodes).

screenshot-01

Demo
Checkout feature branch feature:load-incomplete-members and edit the following relations to demonstrate typical relation member sizes:

Discussion

  • Impact on performance
    • According to OSM Wiki, relations are not recommended to have more than 300 members. But of course, contributors may choose to ignore this guideline
  • Letting user complete one relation member at a time
    • Instead of loading all of the relation members at one time, another possibility would be to load one at a time
    • But currently, there is no hint as to the full content of the incomplete member. In other words, user would only be shown <not downloaded> at first and possibly the role. The user would have to load each one of them until he/she finds the member that he/she is looking for.
  • Placement of button
    • Currently, the enhancement has the button next to the form-label of a member, which might suggest that the action to be taken would only affect one member.
    • A better placement would be somewhere else where it suggests that the action will affect all members. (Like 'Complete All Members' or something to that affect)

Notes

  • The buttons are currently styled with an interim icon ('geocode').
  • jshint OK jshint js/id/ui/raw_member_editor.js

sogko added 3 commits July 11, 2014 08:18
# Implementation details:

## iD.ui.RawMemberEditor
For each member in 'incomplete' state, a button is shown that allows user to re-load the relation and complete its members.
This is done through context.connection().loadEntity().
@pnorman
Copy link
Contributor

pnorman commented Jul 11, 2014

So, performance wise, it depends on the API calls used

Both the relation full and way full calls have been moved to cgimap and are much faster than they used to be. Most wiki documentation probably pre-dates that, not that over 300 members on a route relation is a good idea, but the issues for those downloading them are much less.

The boundary for Germany, pretty much a worst case, took me under 30 seconds to download in testing. It used to time out at 5 minutes.

For loading relation members, there are three strategies to get all of a relation

  • Use the relation/full call.
  • Use the way/full call on each way
  • Use the ways? call on all the ways, then fetch the missing nodes with the nodes? call.

Given the move to cgimap, I recommend the first. The others are only worth it if you already have most of the relation, something that will not be the case for big relations. For small relations, the overhead of relation/full is better, only having one API call.

@sogko
Copy link
Author

sogko commented Jul 11, 2014

@pnorman Thanks for the insight! Yeah you're definitely right, API calls does feel speedy. So it does make sense to get the full relation call while not being too worried of downloading massive data, to a certain extent i guess. (Question: for boundary for Germany, how large of a download are we talking about here?)

Though I have another concern with performance in terms of entities loaded into the iD.Graph.
(I just started going through the codebase, so feel free to correct me or add if I missed something).

From what I understand, connection().loadEntity() eventually triggers on('load.context') which merges the new data into the graph loaded in the client browser.

So would there be any concerns with accumulatively loading massive chunk of data onto the graph in terms of client performance (browser slowdowns/crash)? Or is that a matter of restarting the browser if that happens?

So far in my usage, I don't feel any penalty but I may have not used it for long enough.

Thanks!

@jfirebaugh
Copy link
Member

Hey @sogko, thanks for working on this. This would be a nice feature and the approaches you and @pnorman have been discussing sound pretty good.

So would there be any concerns with accumulatively loading massive chunk of data onto the graph in terms of client performance (browser slowdowns/crash)?

There could be. It would be good to look at some numbers from heap profiling of large relations.

@bhousel
Copy link
Member

bhousel commented Feb 17, 2016

This PR has gone a bit stale - @sogko are you still interested in merging this? I think it would be a really useful feature.

Are there any outstanding issues (other than changing the icon) that are holding this up? I could replace the icon with something that looks more "download-y".

@homersimpsons
Copy link
Contributor

Any news on this ?

@bhousel
Copy link
Member

bhousel commented Jul 1, 2017

Any news on this ?

Nope, news would be here.

@bhousel
Copy link
Member

bhousel commented Oct 11, 2018

superseded by #5396

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants