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

External layers #42

Merged
merged 4 commits into from
Jun 29, 2018
Merged

External layers #42

merged 4 commits into from
Jun 29, 2018

Conversation

yourealwaysbe
Copy link
Contributor

Here is a proposal implemention of an option for track waypoint layers which allows them to be stored in external GPX files rather than inside the .vik file. The pull request is in four commits (since you may not like all features):

  • Addition of a file save dialog to uibuilder (as opposed to file open)
  • Basic external layer implementation
  • Implementation of lazy loading of external layers
  • Addition of "Open GPX as external layer" to the file menu

My motivation for adding this is due to the way i plan cycle routes. I have one huge file planning.vik with all routes i've planned or found elsewhere, as well as all cafes and pubs from OSM. This makes it easy to chop up old routes into new ones and find places to stop. I also have all.vik which contains all the rides i've recorded, mainly for my own amusement. These files are stored in a git repository for when i switch between machines.

External layers are useful here because:

  • Git conflicts are unwieldy since they spread across hundred or thousands of lines. With the data stored in external files, these conflicts should be less of an issue as only the metadata needs to be merged.
  • The routes need to be exported to or imported from GPX files anyway, so i end up with duplicated data lying around.
  • Lazy loading means i don't have to wait for large chunks of data to load if i'm not going to view or use them.

The main issue with this is that some track options don't get exported to GPX. I've only noticed track colour having this issue so far. With lazy loading, summary statistics in parent layers will be off if they have unloaded children (this is not something that bothers me).

You can create an external layer either by visiting the new "Filesystem" group in the track waypoint layer properties dialog, or by using the "Open GPX as external layer" option in the file menu.

@matthewhague
Copy link

matthewhague commented May 17, 2018

I've fixed a bug in writing to files (i think a commit got lost from a while ago).
I've also noticed that the export function writes the GPX files in a slightly different order each time i save. This is not ideal. I will look into it.

Update: This looks like it is due to the gpx export preference being by time. Perhaps the best solution is to add an option to the export function to override the export order in the prefs (and always use name).

@rnorris
Copy link
Collaborator

rnorris commented May 17, 2018

This looks very interesting thanks. I'll probably look to merge all of it.

However it doesn't compile for me, I think there's a couple of issues with commit d83e5c6.

For the new function trw_write_file_external () the error msg tries to use 'extfile' - which doesn't exist - it probably should use 'trw->external_file'.
It then also tries to free 'extfile_full', which also doesn't exist. At first glance I don't think anything needs to be freed here.

For the 'OpenExtLayer' menu option in vikwindow.c, it would be better to use an unused letter e.g. 'La_yer' as the x in 'E_xternal' clashes with the 'E_xit' for the keyboard accelerator.

I'm wondering how far to take the external load concept as two ideas leap out at me.

  1. Extend the load/import method to select multiple files.
    ATM the Open as External can only load one at a time.
    If extending the GUI method to use lists is hard/time consuming to do, it would probably OK to have as an command line parameter flag to open subsequent files as external layers.
  2. Aggregate layers (and/or Tracks too) could be saved in separate .vik files themselves.

My main record of interesting cycling & walking efforts (i.e. not commuting) is currently ~170Mb and takes about 10 seconds to load from an SSD device. Most of the things are invisible and then Aggregate groups are turned off/on depending on what I want to look at. Splitting it into separate files for these should make it much faster to load & save too.

I think a manual 'force load' GUI option would be very useful - to override the Lazy Load so that Summary Statistics and so forth would be usable when desired.

@yourealwaysbe
Copy link
Contributor Author

Apologies for the failing compile. Not sure what happened there (it worked on my machine, i swear!). I pushed a fix for that. I'll respond to the rest tomorrow.

@yourealwaysbe
Copy link
Contributor Author

yourealwaysbe commented May 18, 2018

I've pushed several more commits (this feature is turning out more experimental than i first thought). Some of these are bug fixes (lazy load would duplicate data in some cases, change shortcut for file menu option). Others are new features. First i changed timestamp ordering to fallback on name (for reasons explained in the commit message).

Second there are now three types of layer: internal, external, and external-no-write. The latter is the default when GPX files are opened through the file menu. External-no-write means the file is loaded (lazily) but not exported when a save occurs. This prevents the viking export overwriting any data in the GPX file that it does not recognise (e.g. <extensions><speed>). I've added two new trw layer icons to help the user distinguish easily which kind of layer they're dealing with. (Oh -- i need to write help documentation for this.)

@rnorris:

Extend the load/import method to select multiple files.

How would this work? Loading multiple files is straightforward conceptually, but where should any changes be written back? This option might work with external-no-write layers?

Aggregate layers (and/or Tracks too) could be saved in separate .vik files themselves.

This is a nice idea. One could also support other types of data formal like kml/kmx. I'm reluctant to make any commitments to doing it though.

I think a manual 'force load' GUI option would be very useful - to override the Lazy Load so that Summary Statistics and so forth would be usable when desired.

Adding an "always load layer" option would be very easy. I suspect you mean something like an option on the layer context menu that means that the entire subtree of the current layer will be loaded. I'm happy to do either, but the latter would need a few days for me to find time.

@rnorris
Copy link
Collaborator

rnorris commented May 22, 2018

    Extend the load/import method to select multiple files.

How would this work? Loading multiple files is straightforward conceptually, but where should any changes be written back? This option might work with external-no-write layers?

Yes - automatically to the 'external-no-write'.
Thus if one has a load of existing .gpxs, the .vik can be just a container for these other ones.

I suspect you mean something like an option on the layer context menu that means that the entire subtree of the current layer will be loaded.

Yes that would be great.

@yourealwaysbe
Copy link
Contributor Author

I've added an option to the aggregate layers for loading all external layers cotained therein.

@rnorris
Copy link
Collaborator

rnorris commented May 26, 2018

I'm a little unsure about the Track Colour, this is not in the GPX1.0 standard.
I thought I had already written something, but it remained private as it was the start of some GPX1.1 support.
So I've just uploaded this work here:
https://github.com/rnorris/viking/tree/GPX1.1

This uses "gpxx:TrackExtensiongpxx:DisplayColor..." and a text name for the colour - e.g. 'red'.
This works with Garmin GPSs.

This needs some more thought about how GPX1.1 & part should be handled for the future.

Otherwise is it possible to take out the color commit + rework this branch to fixup/squash the 'Fix bugs...' commits into the original commit to make a clean set of commits?
I often use "git rebase -i HEAD~n" and then inactively merge development bug fixing into the appropriate commit(s) before releasing into the main branch.

IMHO This makes the commit history more logical to follow for the future.
Thanks.

@yourealwaysbe
Copy link
Contributor Author

yourealwaysbe commented May 26, 2018 via email

@rnorris
Copy link
Collaborator

rnorris commented May 26, 2018

I believe you can re upload the same branch from the command line using '--force' option.
I don't know what happens to these comments on github, but they aren't important.

@yourealwaysbe
Copy link
Contributor Author

yourealwaysbe commented May 28, 2018

Think it's done now. I made a bit of a mess of it at first. Below are my older comments.

Old comments:

This is the result of a rebase and force push. I've squashed it into three commits. The force-push seems to have attributed previous commits to both of us. If this is expected/not an issue, then that's ok i guess, else i can try to put together something cleaner.

Ugh: i realise this is because i used rebase with HEAD~'some large enough number' rather than including only my commits. Looking for a good solution now.

Instead of the usual file open option, add a new widget for when you
want to specify a file to save to.  This allows file names to be typed
(using GTK's file save action) and checks whether the file exists.  If
it exists it prompts the user whether they want to overwrite.

To implement this the return type of the callback for the vik file entry
functions had to be changed to allow a boolean saying whether to accept
or whether to ask again.

This is introduced for the external layers feature, but committed
separately as it provides general functionality.
When exporting to GPX timestamp is the default ordering.  However, if
tracks are converted from routes, they have no timestamp information, so
the order is unstable between multiple import/exports.  Including name
as a fallback will introduce some stability.

If two tracks have the same name, then somehow the user has chosen to
make these layers hard to distinguish.
@rnorris
Copy link
Collaborator

rnorris commented May 28, 2018

OK getting there! The rebase looks good to me.

But found some further issues from some quick testing:

  1. After loading in a file via "Open GPX as External _Layer...", the layer ends up with a normal TRW Icon.
    It should have the new 'lock' icon.
    Probably want to make trw_layer_replace_external() call trw_update_layer_icon() to get the correct icon shown.

  2. If one loads a GPX normally, then changes the Filesystem->External Layer to Yes or No Write and then upon save & reload in a .vik file - it won't know where to load it from.
    This is since the 'Save layer as' field is empty.
    Would it be a good idea to always store the initial filename of where the GPX came from into the 'Save layer as' field?
    (This might require rework to existing code to flow the filename into the layer properties - so you may decide its currently not worth the effort to make it happen)

  3. As a extra change - perhaps the sensitivity of the 'Save layer as' text entry can be disabled when the External layer is set to No.
    NB the hacky way to do this is demonstrated in vikmapslayer.c -> maps_layer_change_param() usage

Thanks for you continued work

Allows the dialog to be opened by the program.
@yourealwaysbe
Copy link
Contributor Author

Apologies for the delay. I've fixed the icon issue.

For the blank file location issue i've done the following: when the layer is changed to external but has a blank file entry, the file save dialog is forced to open. This way the user is saved a click and prompted to choose a save location. They can cancel the dialog and leave things blank if they wish, but then it's kind of their own fault. Changing to external no write is weird from the properties dialog (hence the open as external in the file menu). In particular, you choose a file that will only be read once the vik file is reloaded. We could force a read here, but then the original layer contents would have to be overwritten (perhaps this is natural? we could do this if the user accepts a warning dialog?). We could force a save, but maybe the user doesn't want to overwrite the file (especially since they chose "no write").

For this, i've made the choose_file function from vikfileentry a non-static one, so that it can be called from viktrwlayer.

Option to store a TRW layer in a GPX file rather than in the .vik file.
This can be specified in the properties dialog of the TRW layer.  Adds
two new properties to the .vik file to specify the layer type and
external file name.

External GPX layers are only loaded if they are visible.  The saves on
file loading times when external files are large.  If a layer is drawn
(made visible) or selected, then it is loaded.  The disadvantage of this
is that summary statistics for parent layers will pot take account of
external layers that are not loaded.

Also adds menu option for open external layer: create a new layer that
opens a GPX file and maintains an external reference to it as an
external layer (rather than importing all the data points into the vik
file).

TRW layers can now be internal, external, or "external no write".  In
the latter, data is loaded from the file but not written back.  E.g. if
you open a GPX recording, you probably won't edit it, but would be
annoyed if you saved your .vik file and the GPX export stripped data
(like GPS speed) from your .gpx file.  Hence if you open a file as an
external layer, it's no write by default.

Also adds right-click menu option to aggregate layers which recurses
through all children and loads all external layers.
@yourealwaysbe
Copy link
Contributor Author

I've changed the behaviour again. If you haven't previously specified an external file name, the "no write" option is not available. This way, through the layer properties you're really saying "i want to save to this file". If you have an external layer already saved to a file you can select "no write" to say "no more changes". If you want to create an external layer from an existing file, you go through the file->open as external option.

This isn't fully foolproof in that you can open the dialog twice to subvert the restrictions (like you can press cancel on the save as dialog), but at least it guides the user.

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.

3 participants