Bug 192948 - Change SaveAction to work with ANY selectionType, not EXACTLY_ONE
Change SaveAction to work with ANY selectionType, not EXACTLY_ONE
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Actions
7.0
All All
: P1 (vote)
: 7.0
Assigned To: Jaroslav Tulach
issues@platform
: API, API_REVIEW_FAST, NETFIX, UI
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-07 00:38 UTC by krissco
Modified: 2011-01-04 16:19 UTC (History)
4 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
patch (5.94 KB, patch)
2010-12-07 00:39 UTC, krissco
Details | Diff
New patch (5.72 KB, patch)
2010-12-10 15:36 UTC, krissco
Details | Diff
patch (8.67 KB, patch)
2010-12-13 18:54 UTC, krissco
Details | Diff
patch (8.82 KB, patch)
2010-12-13 21:15 UTC, krissco
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description krissco 2010-12-07 00:38:04 UTC
SaveAction is written to only allow a single instance of SaveCookie.
This patch extends the functionality to allow multi-selection to work.

Note: There is something missing in this patch! I'm not sure where the layer file is for SaveAction - perhaps it's generated from the source somehow? If the layer file exists in another module, it should have its selectionType attribute changed to ANY.

This is my first patch, give me instruction where needed. Thanks.
Comment 1 krissco 2010-12-07 00:39:08 UTC
Created attachment 103649 [details]
patch
Comment 2 Jaroslav Tulach 2010-12-07 08:13:32 UTC
Good work Krissco! Please enhance your patch so it has additional attributes:

Y01 Write a unit test that fails before your code change and succeeds after it.
Y02 Increase spec version in openide.actions/manifest.mf
Y03 Modify javadoc of SaveAction to mention that since the new spec version it handles ANY.
Y04 Add new entry to openide.actions/apichanges.xml

If these changes are made and other apireviewers don't disagree with the change, I'll integrate your new patch on Dec 14, 2010.
Comment 3 Jiri Kovalsky 2010-12-07 10:01:12 UTC
Added to the list [1] of community contributed bug fixes.

[1] http://wiki.netbeans.org/NetFIXIssues
Comment 4 Jesse Glick 2010-12-08 14:10:20 UTC
http://deadlock.netbeans.org/hudson/job/nbms-and-javadoc/lastSuccessfulBuild/artifact/nbbuild/build/generated/layers.txt shows:

o.n.core.ui    Actions/System/o-o-actions-SaveAction.instance

where the layer has

<file name="org-openide-actions-SaveAction.instance">
    <attr name='instanceCreate' methodvalue='org.openide.awt.Actions.context'/>
    <attr name='delegate' newvalue='org.openide.actions.SaveAction'/>
    <attr name="selectionType" stringvalue="EXACTLY_ONE"/>
    <attr name='surviveFocusChange' boolvalue="false"/>
    <attr name='displayName' bundlevalue='org/openide/actions/Bundle#Save'/>
    <attr name='noIconInMenu' boolvalue="true"/>
    <attr name="iconBase" stringvalue='org/openide/resources/actions/save.png'/>
    <attr name="type" stringvalue="org.openide.cookies.SaveCookie"/>
</file>

(Quite a mouthful; would be better replaced by annotations in SaveAction.java!)

As to whether we _want_ to change the default registration of SaveAction, I'm not sure; this is a behavioral change visible in the UI. I guess it is harmless and might sometimes be appreciated, though most people would be using Save All instead. UI designers might prefer the mode to be ALL (which I guess this patch also supports?).


[JG01] Please do not include gratuitous reformatting in a patch such as:

-* @see SaveCookie
-*
-* @author   Jan Jancura, Petr Hamernik, Ian Formanek, Dafe Simonek
-*/
+ * @see SaveCookie
+ *
+ * @author   Jan Jancura, Petr Hamernik, Ian Formanek, Dafe Simonek
+ */

What if I fix the registration of SaveAction today to use an annotation? Your patch will suddenly fail to apply, even though you are not doing anything important to the class signature. Anyway the less there is to review, the happier reviewers are; and the shorter the final changeset, the easier it is to track down regressions using 'hg annotate'. We encourage use of MQ (or pbranch) for creating API review patches, specifically so you can trim down the reviewable diff to the minimum.


[JG02] Why does your patch remove the use of UserQuestionException? While this might be fine on its own (AFAIK UQE is no longer ever thrown in this circumstance), it seems completely unrelated to supporting ANY. Removing this change means your patch need not touch performAction at all.


[JG03] Trivial, but the cookieFromNode helper var would not be needed if the break statement was changed to 'continue COOKIE;' where 'COOKIE: for (SaveCookie saveCookie ...) {...}'. Might be slightly clearer.
Comment 5 krissco 2010-12-10 15:34:33 UTC
Thanks for the input.

KS01 Regarding Y01, I'm not entirely sure how to do this. Should I replace the existing SaveActionTest or create a new class for this test?

KS02 Regarding Y04, check over the (new) patch, I think I did it right.

KS03 JG01 is a good point, don't know how that got in there.

KS04 JG02 I guess I was a little over-zealous with the refactoring. To clean up that code, would I create a separate issue?

KS05 JG03 Nice! I'm sure that I've NEVER used a labeled continue before.

KS06 Regarding the layer file, how is that created? Is it something that I need to modify in the patch?
Comment 6 krissco 2010-12-10 15:36:46 UTC
Created attachment 103942 [details]
New patch
Comment 7 krissco 2010-12-10 15:44:01 UTC
Comment on attachment 103942 [details]
New patch

Looks like I ended up with more extraneous changes in the apichanges.xml.
Comment 8 Jesse Glick 2010-12-10 15:50:46 UTC
(In reply to comment #5)
> Should I replace the
> existing SaveActionTest or create a new class for this test?

Probably enough to add new test cases to that suite?

> KS04 JG02 I guess I was a little over-zealous with the refactoring. To clean up
> that code, would I create a separate issue?

Yes please.

> KS06 Regarding the layer file, how is that created? Is it something that I need
> to modify in the patch?

Currently the registration is in core.ui/src/org/netbeans/core/ui/resources/layer.xml so your patch could modify that. (I investigated converting this to @ActionRegistration but in this case it seems impossible for complex reasons.)
Comment 9 krissco 2010-12-13 18:54:54 UTC
Created attachment 104024 [details]
patch

I've modified the layer file core.ui/src/org/netbeans/core/ui/resources/layer.xml. I also looked at annotations, but getting the Node name (for output) from the SaveCookie would be an issue.

KS07 I've added a test to satisfy Y01. I'm not very familiar with JUnit and I have some concerns about concurrency. What is the recommended route, should I write the test to handle concurrent operations (imagine that SaveAction is changed to asynchronous by a future API change)? If so, what is the preferred idiom - synchronized blocks, atomic objects, etc.? Is it okay to share the cnt variable with the other test? Let me know your recommendations.

KS08 I've been following the instructions provided here http://netbeans.org/community/contribute/patches.html. I still seem to be getting some extraneous changes in the diff file. Is this a big deal, and what can I do about it?
Comment 10 krissco 2010-12-13 21:15:50 UTC
Created attachment 104030 [details]
patch

Fixes a regression in the last patch (return from mode() method).
Comment 11 krissco 2010-12-17 07:01:57 UTC
Disregard KS08 - I was removing trailing whitespace characters.

I'm interested in getting this integrated into a build. Please let me know if there is anything else that I need to do.
Comment 12 Jaroslav Tulach 2010-12-17 13:36:18 UTC
The logic in performAction(Lookup) would probably deserve more tests, but otherwise I am fine with the change. I'll integrate it before Christmas 2010.
Comment 13 Jaroslav Tulach 2010-12-20 21:49:41 UTC
ergonomics#1d4ba9faa6f4
Comment 14 Quality Engineering 2010-12-23 07:04:31 UTC
Integrated into 'main-golden', will be available in build *201012230001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/1d4ba9faa6f4
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #192948: SaveAction supports multiselection now. Contributed by krissco@netbeans.org
Comment 15 Jiri Kovalsky 2011-01-04 16:19:07 UTC
Great job guys. Thanks Jardo for the review & integration and more importantly thanks to Kris for the patch(es) and especially persistence!


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo