There are a few blockers in my review. The basics, code comments and findbugz type of stuff, hold up well. But there are issues when using it.
First, the documentation isn't enough to get going. It doesn't explain the differences between file/link, or Feature Document vs. Feature Attachement vs. Shapefile Documents, which makes the document describing the mechanics of setting these confusing. I think there needs to be an overview section to make it clear what these things are. There are also a number of issues: * Feature Document uses relative paths only. When my test shapefile had an absolute path it was appended to the path of the shapefile. If this is expected behaviour it needs to be made considerably more clear. * The documentation on how to turn enable documents "Configure hotlink attributes" isn't quite clear enough. I was completely unable to get it working without pulling Naz over. Having said that, once I had done it once, it made sense, so I believe it's a doco issue not a usability issue. For example, only selecting in the catalog, not in the layers view, is a pain. But the needed menu is not called "GeoResource Properties" and is only available under the Data menu, not the context menu. It should also be stated that a feature needs to be selected with the feature selection tool. I know it should be self-evident, but since there were changes in the info tool that was my first stop. Since Box selection was higher on the list that was my second. * To make matters worse, there's a 'Resource Properties" menu under the Data menu when a layer is selected in the Layers view, but it doesn't have the Document page. * I also got a strange issue when trying to change a Feature Attachment link in my test data, where the geometry of the selected feature was displayed instead of the link url (which was correctly displayed in the Document view). On 9 August 2012 16:35, Jody Garnett <[email protected]> wrote: > Thanks Mark: > > There are three main things to look at around Menu enablement. They are not > really new to this document_source branch; but the fix for the issue is new. > > 1) net.refractions.udig.catalog.ui > > IServicePropertiesCommandHandler has been rewritten to bring up only the > property pages for IService. You can compare against the implementation of > IGeoResourcePropertiesCommandHandler which I have not fixed (until I get a > code review). > > 2) net.refractions.udig.tool.info/plugin.xml > > Makes use of a *new* core.expression property (this is the first time I have > made one!). The property is "net.refractions.udig.catalog.featureSource" and > can be used to verify that an IResolve has access to a FeatureSource before > letting that page be shown. > > <page > category="net.refractions.udig.catalog.ui.property.resourcePage" > > class="net.refractions.udig.catalog.document.DocumentPropertyPage" > icon="icons/obj16/file_doc_obj.png" > id="net.refractions.udig.catalog.document.property.documentPage" > name="%documentPage.name"> > <enabledWhen> > <and> > <or> > <instanceof > value="net.refractions.udig.catalog.IGeoResource"> > </instanceof> > <adapt > type="net.refractions.udig.catalog.IGeoResource"> > </adapt> > </or> > <test > property="net.refractions.udig.catalog.featureSource"> > </test> > </and> > </enabledWhen> > </page> > > The actual implementation of the new property is in > net.refractions.udig.catalog plugin, the file CanResolvePropertyTester.java. > I wrote it after checking several other eclipse implementations and thus far > it appears to be working. > > If this code review checks out I will make a similar core.expression for > checking ILayer and friends. > > 3. The DocumentPropertyPage > > Naz already checked this one. > > -- > Jody Garnett > > On Wednesday, 8 August 2012 at 5:06 PM, Mark Leslie wrote: > > I'm out of time for the day, but I'll have a go at it in the morning. > > -- > Mark > > On 8 August 2012 09:15, Nazareno Chan <[email protected]> wrote: > > I’m checking this now as of writing, and can we ask another person to > review/accept the pull request? Since I think it would be biased for me to > do so since I did some of the changes. > > > > Thanks guys. J > > > > From: [email protected] > [mailto:[email protected]] On Behalf Of Jody Garnett > Sent: Wednesday, 8 August 2012 9:06 AM > To: User-friendly Desktop Internet GIS > Subject: [udig-devel] document view stage 2 pull request > > > > Stage 2 pull request is up: > > > > - https://github.com/uDig/udig-platform/pull/118 > > > > This is the next batch of changes for: > > > > http://udig.refractions.net/confluence/display/UDIG/Document+View > > > > This pull request is for stage 2 "Stage 2 General purpose implementation". > > > > This has three major changes: > > 1) PropertyPage for configuring document "hotlinks" (thanks for the review > Naz) > > 2) Fixed up how the Data > Service Properties and Data > Resource Properties > are opened > > - created a core expression property for IResolve so we can test for > "featureSource" etc... > > - used for both menu and property page visibleWhen settings > > 3) Fixed up the commands used to open "Service Properties" by making a > custom selection provider > > > > Naz if you can check this over, I know this is just an step along the way to > completion - the the menu enablement stuff is cool. > > > > Moovida I would like to touch base with you for the final stage 4 pull > request, we should be a week or so away form that. > > > > -- > > Jody Garnett > > > > > ________________________________ > The contents of this email are confidential and may be subject to legal or > professional privilege and copyright. No representation is made that this > email is free of viruses or other defects. If you have received this > communication in error, you may not copy or distribute any part of it or > otherwise disclose its contents to anyone. Please advise the sender of your > incorrect receipt of this correspondence. > > _______________________________________________ > User-friendly Desktop Internet GIS (uDig) > http://udig.refractions.net > http://lists.refractions.net/mailman/listinfo/udig-devel > > _______________________________________________ > User-friendly Desktop Internet GIS (uDig) > http://udig.refractions.net > http://lists.refractions.net/mailman/listinfo/udig-devel > > > > _______________________________________________ > User-friendly Desktop Internet GIS (uDig) > http://udig.refractions.net > http://lists.refractions.net/mailman/listinfo/udig-devel > -- Mark Leslie Geospatial Software Architect LISAsoft ------------------------------------------------------------- Ph: +61 2 8570 5000 Fax: +61 2 8570 5099 Mob: +61 Suite 112, Jones Bay Wharf 19-21 Pirrama Rd Pyrmont NSW 2009 ------------------------------------------------------------- LISAsoft is part of the A2end Group of Companies http://www.ardec.com.au http://www.lisasoft.com http://www.terrapages.com _______________________________________________ User-friendly Desktop Internet GIS (uDig) http://udig.refractions.net http://lists.refractions.net/mailman/listinfo/udig-devel
