Re: Custom layers in PgAdmin IV Mapview

On Tue, Sep 18, 2018, at 17:39, Dave Page wrote:
> Hi
>
> On Tue, Sep 18, 2018 at 11:09 AM, Atle Frenvik Sveen <atle(at)frenviksveen(dot)net>
> wrote:
>
> > Hello again!
> >
> > I think I've accomplished this task now, see last screenshot and attached
> > patch.
> >
> > I hope I did this patch right?
> > git checkout master
> > git fetch upstream
> > git merge upstream/master
> > git checkout mapviewer_custom_layers_3646
> > git merge master
> > mapviewer_custom_layers_3646
> > git format-patch master --stdout > RM3645.patch
> >
>
> Looks right I think - though something got missed as the resulting patch
> doesn't actually apply:
>
> dpage(at)hal:*~/git/pgadmin4*$ git apply ~/Downloads/RM3645.patch
>
> /Users/dpage/Downloads/RM3645.patch:1919: trailing whitespace.
>
> .fa-clickable {
>
> /Users/dpage/Downloads/RM3645.patch:1920: trailing whitespace.
>
> cursor: pointer;
>
> /Users/dpage/Downloads/RM3645.patch:2062: trailing whitespace.
>
> .fa-clickable {
>
> /Users/dpage/Downloads/RM3645.patch:2063: trailing whitespace.
>
> cursor: pointer;
>
> error: patch failed: web/package.json:69
>
> error: web/package.json: patch does not apply
>
> error: patch failed: web/yarn.lock:31
>
> error: web/yarn.lock: patch does not apply
>
Oh. I honestly have no idea what happened here, but I'll see what I can do.

>
> >
> > The OrderedList preferences type seems to work fine, and the layout works
> > okay by my standards.
> > - You are able to edit existing entries (but elements that are marked
> > required but not filled in are reverted)
> > - You are able to add a new entry (but all required elements must be
> > filled before it's saved)
> > - Elements can be deleted (I was thinking about disallowing the "default"
> > entries, but not sure if this makes sense)
> > - Elements can be re-ordered (first in list is default)
> >
>
> Couple of fixes we really need for UI consistency:
>
> - The up/down buttons should be FontAwesome icons, following the same
> formatting as the delete icon.

These buttons are fontawesome, but I guess there are multiple styles?

> - The Add button should be at the right hand end of the (missing) table
> header, with just the + icon in it (see the Privileges or Security Labels
> tables on the Security tab of the Database properties dialogue for an
> example).
>
Got you. I moved it to the left, as the right hand of the table is hidden behind a horizontal scrollbar on smaller window-sizes. But the app is probably meant to be used in full size? Also had some issues with using the style used for the "top header" in the other tables. But I'll give it another try!

> >
> > The maviewer seems to pick up the preferences just fine, and the default
> > preferences are entered as best as I could (mainly replicating the
> > hard-coded layers).
> >
> > Some improvement-points I guess:
> > - You can add several new elements without filling them in (though they
> > won't be saved)
> >
>
> Some of the existing tables allow that, so I don't think that's the end of
> the world.
>
>
> > - No validation of wether the TileUrl is valid (but this is a difficult
> > task, not worth it?)
> >
>
> I think that's fine - but we should have good documentation in
> preferences.rst to help the user get it right.
>
>
> > - Possibly better explaination (but for me, who knows this, it's difficult
> > to improve on)
> >
>
> I'll review and tweak any strings as part of the the review/commit. As long
> as you put something there that I can understand, I can massage the wording.
>
Great!

> > - I was not able to re-use code everywhere I wanted, so some duplication
> >
>
> To what degree? If it's entire functions, then we should look at factoring
> them out into shared modules somewhere.
>

I guess i mostly glanced at existing code and borrowed parts. Did not feel comfortable trying to refractor code I did not completely understood. So I guess it's okay. But given my lack of familiarity with the code I'll accept a second opinion here.

> > - I was not quite sure about code-style, so feel free to comment on that.
> >
>
> Nothing stands out as being wrong. The main thing is to make it look like
> the rest of the code, so it doesn't appear out of place. There are also
> some basic notes at
> https://www.pgadmin.org/docs/pgadmin4/dev/coding_standards.html
>

Ah, the es-comment makes sense. I guess I've violated the practice of declaring all variables at the to of a function,other than that I tried my best to align with existing code.

> > - This seemes pretty hard to write sane tests for, so I skipped that part.
>
>
> I agree for the actual maps. However, I would like to see tests for the
> preference storage/retrieval as you've added a new type. Aditya and
> Khushboo can give guidance on how to do that if needed (as well as the
> layout).

So tests for the Python-code? As long as there are tests for the other types I think I can figure that out. If not you'll hear from me ;)

> Thanks!

Thanks for the comments!

-a

--
Atle Frenvik Sveen
atle(at)frenviksveen(dot)net
45278689
atlefren.net