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

[GEOS-7516] Resource Browser (ResourceStore GUI) #1572

Closed
wants to merge 1 commit into from

Conversation

NielsCharlier
Copy link
Contributor

@NielsCharlier NielsCharlier commented Apr 18, 2016

GSIP 139 - GeoServer Resource Browser GUI
https://github.com/geoserver/geoserver/wiki/GSIP-139

[GEOS-7516] Resource Browser (ResourceStore GUI)
https://osgeo-org.atlassian.net/browse/GEOS-7516

@jodygarnett
Copy link
Member

We tend not to worry too much about community modules - as long as they do not break the build. They get a review when being considered for extension (or integration with a main module).

That said this pull request also includes changes to core modules, can you split the two pull requests into two please - I would like to have a better look at what you needed to change.

My initial take ResourceAdaptor has been changed to always use absolute paths, which has had a ripple on effect to utility methods and test cases. Can I ask you to start a seperate email thread and we can sort out how to report the issue you are fixing and an appropriate fix.

@NielsCharlier
Copy link
Contributor Author

NielsCharlier commented Apr 19, 2016

Jody, the first commit has a separate PR and JIRA issue #1565. https://osgeo-org.atlassian.net/browse/GEOS-7493 where it can be discussed.

The commit is just on this branch as well because I needed it to work.

@jodygarnett
Copy link
Member

Remove JAI from the Mac java extensions for the user. See other email
thread for the path.
On Tue, Apr 19, 2016 at 1:13 AM NielsCharlier notifications@github.com
wrote:

Jody, the first commit has a separate PR and JIRA issue #1565
#1565.

The commit is just on this branch as well because I needed it to work.
https://osgeo-org.atlassian.net/browse/GEOS-7493 It can be discussed there


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#1572 (comment)

Jody Garnett

@bencaradocdavies bencaradocdavies self-assigned this Apr 20, 2016
@bencaradocdavies
Copy link
Contributor

The other PR #1565 is merged.

@bencaradocdavies
Copy link
Contributor

@NielsCharlier is there an OSGeo Jira issue for this so that it gets in the release notes?

@NielsCharlier
Copy link
Contributor Author

@jodygarnett Jody I don't understand your comment and I do not know what you are referring to.

@NielsCharlier NielsCharlier changed the title Resource Browser (ResourceStore GUI) GEOS-7516 Resource Browser (ResourceStore GUI) Apr 21, 2016
@bencaradocdavies
Copy link
Contributor

bencaradocdavies commented Apr 21, 2016

@NielsCharlier thanks for the Jira issue. I added a link in the PR description.

@bencaradocdavies bencaradocdavies changed the title GEOS-7516 Resource Browser (ResourceStore GUI) [GEOS-7516] Resource Browser (ResourceStore GUI) Apr 21, 2016
@bencaradocdavies
Copy link
Contributor

@NielsCharlier this PR now has conflicts. Please update it.

<parent>
<groupId>org.geoserver</groupId>
<artifactId>community</artifactId>
<version>2.9-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NielsCharlier the parent version must now be 2.10-SNAPSHOT as the 2.9.x branch was created when 2.9-RC1 was released, while you were waiting for me to review this issue. Apologies for the inconvenience.


.menu a #delete {
background-image: url("icons/delete.png");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line at end of file.

@bencaradocdavies
Copy link
Contributor

OK, @NielsCharlier I am going to stop whinging about the missing newlines at the end of all your test resources. Please fix them all to improve maintainability. Many text editors add missing newlines at the end causing spurious commits.

@bencaradocdavies
Copy link
Contributor

bencaradocdavies commented May 23, 2016

@NielsCharlier, what is the source of the icons and what is their licence? This must be documented.

@bencaradocdavies
Copy link
Contributor

@NielsCharlier every other extension I could find except charts and vectortiles uses the singular not plural form. This is I think a great plugin that is destined for promotion. I would love it (and its profile) to be renamed from web-resources to web-resource.

@bencaradocdavies
Copy link
Contributor

bencaradocdavies commented May 23, 2016

@NielsCharlier, other than my truckload of small comments and questions, this is looking very good: clean and thoroughly tested. Not much character development, but a gripping read. It also builds cleanly (with pom version fixes applied locally). I will now start manual testing and report back. I also want to compare it to the GSIP, which although is not strictly needed for a community module, elicited a great deal of good quality feedback.

@bencaradocdavies
Copy link
Contributor

@NielsCharlier I have done some manual testing and recorded my findings here:
https://osgeo-org.atlassian.net/browse/GEOS-7516

<div wicket:id="dialog"></div>

<div class="menu-wrapper">
<div class="menu">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many tabs here, mixing tabs and spaces.

@NielsCharlier
Copy link
Contributor Author

@bencaradocdavies this is pending review again (there is no rush, but making sure you are aware ;) )

@bencaradocdavies
Copy link
Contributor

@NielsCharlier this one is slowly moving up my to-do list. :-)

@jodygarnett
Copy link
Member

How is your todo list looking @bencaradocdavies :)

@bencaradocdavies
Copy link
Contributor

@jodygarnett I should be able to complete reviewing this PR in the next couple of days.

@bencaradocdavies
Copy link
Contributor

Working on this now. There are a couple of tiny pom conflicts with the ncwms commit; I will fix them.

.treeview {
height: calc(100vh - 325px);
width: 100%;
overflow-y: scroll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NielsCharlier you fixed most tabs but sneaked in a few more. :-)

I will fix before merging as I have to deal the pom conflicts.

@bencaradocdavies
Copy link
Contributor

bencaradocdavies commented Jun 18, 2016

@NielsCharlier that looks great. Very thorough work. I think you fixed every single thing I mentioned!

A full build passed, and I did a bit more manual testing. I think your new always-visible toolbar is a big usability improvement as it can be accessed without scrolling when the tree view is expanded.

The additional tiny changes I made were:

  • Fixed pom merge conflicts (@aaime merged ncwms while you were waiting for me)
  • Removed one new trailing tab and converted two new tabs to spaces
  • Added "htm" to the list of textual format file name extensions

I have pushed the resulting commit to master:
2d2c47a

My only unanswered question is the source of the icons, which would be nice to know as their provenance should be documented.

Thanks again for your patience.

Kind regards,
Ben.

@NielsCharlier
Copy link
Contributor Author

@bencaradocdavies thanks! Considering the icons, unfortunately I cannot find where I got them any more, I believe they were creativecommons / free for commercial use but I lost the source. Should they perhaps be replaced by other icons from which we know the source?

@bencaradocdavies
Copy link
Contributor

@NielsCharlier I agree that these icons must be replaced with icons whose source is known. The licence must be GPL-compatible, including use for any purpose and the right to modify and redistribute modifications. CC BY is OK, I think any CC NC or CC ND cannot be used. Not sure about CC SA. CC suggest that it is compatible: https://wiki.creativecommons.org/wiki/ShareAlike_compatibility_analysis:_GPL

@jodygarnett
Copy link
Member

Where are the icons, can I draw us something that matches our "geoslik" style?

@bencaradocdavies
Copy link
Contributor

@jodygarnett you can see the icons in this pull request or you can browse them as merged on master:
https://github.com/geoserver/geoserver/tree/master/src/community/web-resource/src/main/resources/org/geoserver/web/resources/icons

I welcome contributions from those with artistic talent or aspirations. :-)

@NielsCharlier
Copy link
Contributor Author

Absolutely!

@NielsCharlier
Copy link
Contributor Author

So Jody are you doing this (no point in me downloading new ones then)?

@jodygarnett
Copy link
Member

Reviewing that list I see no geospatial specific icons, so we should pull from geosilk.

The geosilk includes geosilk and silk icons.

The original silk project is under a reative Commons Attribution 2.5 or 3.0 License with the following requirements:

As an author, I would appreciate a reference to my authorship of the Silk icon set contents within a readme file or equivalent documentation for the software which includes the set or a subset of the icons contained within.

Will update with what I find/draw.

@NielsCharlier NielsCharlier deleted the webresources branch August 16, 2016 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants