Created attachment 318997 [details]
Video of opening a 20Mb geojson file

It would be more useful to have a progress bar with a cancel button.

(In reply to tobias from comment #2)
> It would be more useful to have a progress bar with a cancel button.

Agreed, but we can not do async loading of these files, so we are screwed :)

How about something like this?
https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/maps/custom-map-largefile.png

The problem with a cut-off limit is, that it's hard to fined one. If you have e.g. a i7 with a fast SSD the limit is far away from a very slow or just old system. 

@Jonas
Does it mean it isn't even possible to make a throbber and a cancel button.

Created attachment 321239 [details] [review]
layersPopover: Add user confirmation when opening large KML/GeoJSON files

When opening KML/GeoJSON files larger than 20MB, the user
will be warned about the long load time, and will be
able to skip the file.

Review of attachment 321239 [details] [review]:

Thanks Razvan!

However I wonder if this is not the wrong place for this? This only applies for files opened by the file chooser. We can also open layers from the command line and by drag and drop, and I would like this warning for all cases. Maybe this belongs in openShapeLayers function, or maybe in the shapeLayer.newFromFile function?

::: src/layersPopover.js
@@ +24,3 @@
 const Utils = imports.utils;
 
+const FILE_SIZE_LIMIT = 20 * 1024 * 1024;

Pls provide a comment of the unit for this.

@@ +129,3 @@
+                        modal: true,
+                        title: _("Warning!"),
+                    let confirmationDialog = new Gtk.MessageDialog({

Writing 20MB here makes it so we cannot change the constant above without needing re-translation in all languages.

@@ +131,3 @@
+                        text: fileName + _(" exceeds 20MB in size. ") +
+                            _("Loading it might slow down the application. ") +
+                        buttons: Gtk.ButtonsType.OK_CANCEL,

I think we need to use this syntax:

text: _("%s ...".format(fileName);

@@ +133,3 @@
+                            _("Press OK to continue loading, or Cancel to skip this file."),
+                    });
+                        message_type: Gtk.MessageType.WARNING,

Pls try to avoid using dialog.run, instead use the following style:

dialog.connect('response', function(dialog, response) {
    if (response === Gtk.ResponseType.OK) {
        ...
    }
    dialog.destroy();
});
dialog.show();

Thanks for the quick response and the great feedback! I guess I rushed to start working on it and I was unaware that layers could be opened through different methods. I'll get back to working on it as soon as possible and I'll touch on all of your suggestions. Also, I'll try to find some good input files that allow for better testing.

(In reply to Razvan Brinzea from comment #8)
> Thanks for the quick response and the great feedback! I guess I rushed to
> start working on it and I was unaware that layers could be opened through
> different methods. I'll get back to working on it as soon as possible and
> I'll touch on all of your suggestions. Also, I'll try to find some good
> input files that allow for better testing.

Np! The code looks great in general! Try opening the large US geojson in geojson.io and exporting it. We have some bug somewhere.

Review of attachment 321239 [details] [review]:

Great first patch Razvan! Some small fixups, but conceptually you have it down perfect.

As Jonas mentioned I think this code is best included in the openShapeLayers function so it works with drag-and-drop too

::: src/layersPopover.js
@@ +24,3 @@
 const Utils = imports.utils;
 
+const FILE_SIZE_LIMIT = 20 * 1024 * 1024;

FILE_LIMIT_BYTES or something would be self documenting too
or FILE_LIMIT_MiB could just be set to 20 and then below in the code we can do * 1024 * 1024

@@ +121,3 @@
         if (fileChooser.run() === Gtk.ResponseType.OK) {
+            fileChooser.get_files().forEach((function(file) {
+                let fileSize = file.query_info("standard::size", 0, null).get_size();

instead of "standard::size" use the constant: 

Gio.FILE_ATTRIBUTE_STANDARD_SIZE

found by looking in gobject-introspection/Gio-2.0.gir

@@ +124,3 @@
+                let fileName = file.get_basename();
+                if (fileSize > FILE_SIZE_LIMIT) {
+                    let confirmationDialog = new Gtk.MessageDialog({

please add the following so the dialog is attached to the main window (you can't drag it around and can't click past it):
                        transient_for: this.get_parent(),

@@ +131,3 @@
+                        text: fileName + _(" exceeds 20MB in size. ") +
+                            _("Loading it might slow down the application. ") +
+                            _("Press OK to continue loading, or Cancel to skip this file."),

_("%s exceeds %s MB in size. Loading it may slow down the application.")

It's good practice to keep translated sentences all in one string. Also a space between a number and its units is the convention. And it's not necessary to describe what OK and Cancel buttons do.

@@ +134,3 @@
+                    });
+                    if (confirmationDialog.run() === Gtk.ResponseType.OK) {
+                        this._mapView.openShapeLayers([file]);

We don't want to show the message once per file, and we want to pass the entire list of files to openShapeLayers because it does some smart things by zooming in so all the layers are shown at once.

Maybe the easiest thing to do is check if any of the files are over 20MB and if any are then show the dialog once. If the user hits cancel, don't load any of the files, and if they hit OK load all the files.

That way it's simple to code and doesn't spam the user with dialogs.

@@ +140,3 @@
+                }
+                else {
+                    this._mapView.openShapeLayers([file]);

here to, we want to pass the entire list of files, not one at a time

Also, there's a large geojson file in https://github.com/JHKennedy4/zip-code-boundaries if you need one to test.

Created attachment 321318 [details] [review]
mapView: Add user confirmation when opening large KML/GeoJSON files

When opening KML/GeoJSON files larger than 15 MB, the user
will be warned about the long load time, and will be
given the possibility to cancel.

Some extra information on the above patch: I took into consideration the feedback from Hashem and Jonas in order to try to get this in a better shape.

I was unable to get rid of the dialog.run() call, because the openShapeLayers function also has a return value which is used somewhere else in the program, and I couldn't find an obvious solution to maintain that functionality and use signals.

There most likely is a solution to use signals and still not break anything, but I haven't been able to see it yet.

Also, I bumped the size limit down to 15 MB for now, because I have a few geoJSON files around the 18-20 MB mark, so this allowed me to do some more testing. It's not necessarily the final value.

Review of attachment 321318 [details] [review]:

Great! Much closer.

Please make sure the first line of the commit message isn't too long: https://wiki.gnome.org/Git/CommitMessages

Also, if you use git bz to attach the patches to this bug you can do git bz -e attach and mark the old patch as obsolete (it does it automatically if the commit message stays the same).

::: src/mapView.js
@@ +223,3 @@
     },
 
+    checkFileSizes: function(files) {

I'd prefix this with an underscore (_checkFileSizes) to convey that the method is only used inside this class.

@@ +226,3 @@
+        let loadFiles = true;
+        for (let index = 0; index < files.length; index++) {
+            let fileName = files[index].get_basename();

If you want to get the fileName the proper way is 

        filename = file.query_info(
            Gio.FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME,
            Gio.FileQueryInfoFlags.NONE,
            null
        ).get_attribute_string(Gio.FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME);
 
(from shapeLayer.js)

@@ +227,3 @@
+        for (let index = 0; index < files.length; index++) {
+            let fileName = files[index].get_basename();
+            let fileSize = files[index].query_info(Gio.FILE_ATTRIBUTE_STANDARD_SIZE, 0, null).get_size();

This line is too long (ideally everything < 80 columns). Maybe:

            let fileSize = files[index].query_info(Gio.FILE_ATTRIBUTE_STANDARD_SIZE,
                                                   0, null).get_size();

@@ +235,3 @@
+                    modal: true,
+                    title: _("Warning!"),
+                    text: _("%s exceeds %d MB in size. Loading it may slow down the application.").format(fileName, FILE_SIZE_LIMIT_MiB),

How about we remove the title and instead have the following?

                    text: _("Loading a large file may slow down the application."),
                    secondary_text: _("%s is larger than %d MB.").format(fileName, FILE_SIZE_LIMIT_MiB)

http://i.imgur.com/SI9mry6.png

I don't know if we even need the secondary_text at all: http://i.imgur.com/CZCkQzs.png

I wonder why the message dialog is shown on top of the file chooser. It looks pretty bad like that.

Maybe using dialog.connect() instead of .run() would help, so that the UI thread can run? Not sure.

To redo openShapeLayers using connect(), you'd have to split it up into 2 functions, openShapeLayers() would call the second (_openShapeLayersInternal maybe?) either if all files are small or in the callback to dialog.connect().

@@ +240,3 @@
+                    loadFiles = false;
+                    confirmationDialog.destroy();
+                    break;

you don't need destroy() or break in this if statement since you have it after the if statement.

@@ +242,3 @@
+                    break;
+                }
+                confirmationDialog.destroy();

When I loaded the zipcodes file the dialog didn't go away. again I wonder if using connect() would help in this regard.

@@ +253,3 @@
         let ret = true;
+
+        if (this.checkFileSizes(files) === true) {

if (this.checkFileSizes(files)) is good enough you don't need to do === true.

Review of attachment 321318 [details] [review]:

Nice, I am however leaning towards wanting this in newFromFile in shapeLayer.js, so that mapView.js does not grow and grow and grow with each new functionality. This i specific to layers, so it could belong in shapeLayer.js, so the check is done for each new file.

Review of attachment 321318 [details] [review]:

Jonas I agree it'd be nice to keep mapView small, but in shapeLayer is pretty solidly part of the "model" and mapView is pretty solidly part of the "view/controller". shapeLayer doesn't even import Gtk.

Follow the principle of least surprise (https://en.wikipedia.org/wiki/Principle_of_least_astonishment), I'd be very surprised if a factory function opened a user-interactive dialog.

Thanks again for the great feedback! I really appreciate you taking so much time to guide me with all of this.

I've resumed work on this but there is one thing that I still can't really figure out. I thought about splitting the code in two functions so that I can use connect(), but I'm afraid I can't find a way for the return value of openShapeLayers to still be correct. 

I made a separate function, _openShapeLayersInternal with part of the code. But if this function gets called by the signal from the dialog box, and if it catches an exception, then I would want openShapeLayers to return false (I've seen the return value used in mainWindow.js). But because I've decoupled these two functions, then openShapeLayers will just finish executing on its own, without any regard as to what happened to the _openShapeLayersInternal function.
Sorry if I'm not very clear, I tend to get lost when trying to explain stuff.

Also, I've read and understood both your points of view regarding as to whether we should have this code, but I am still not very sure where we should ultimately put it.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-maps/issues/39.