Bug 224313 - [api] Create RSE Events for MOVE and COPY holding both source and destination fields
Summary: [api] Create RSE Events for MOVE and COPY holding both source and destination...
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.0 M6   Edit
Assignee: David McKnight CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 222404
  Show dependency tree
 
Reported: 2008-03-27 04:14 EDT by Martin Oberhuber CLA
Modified: 2009-06-17 14:45 EDT (History)
2 users (show)

See Also:


Attachments
patch for describe remote events via operation field (49.66 KB, patch)
2008-03-27 12:47 EDT, David McKnight CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2008-03-27 04:14:36 EDT
+++ This bug was initially created as a clone of Bug #222404 +++

From bug 222404 comment 1: We need extra events in RSE such that a listener gets notified when a remote item is moved or copied. This is needed in order to maintain properties attached to a remote object, see also bug 224199.

Events for MOVE and COPY should hold both the source path and the destination path of the remote object as well as the subsystem(s) involved.

Since MOVE can be seen as a variant of COPY, it might also be possible to handle both cases with a single event.
I need a way to handle setting/managing properties for remote files.
Comment 1 David McKnight CLA 2008-03-27 10:27:18 EDT
Currently, when a move operation takes place we fire the following events:

SYSTEM_REMOTE_RESOURCE_DELETED
SYSTEM_REMOTE_RESOURCE_CREATED

These are the combination of a delete, followed by what we used to do in the old copy action, which is just this:

SYSTEM_REMOTE_RESOURCE_CREATED

However, the old copy action is no longer used (in base rse), since we replaced it with the clipboard copy/paste actions, which make use of SystemDNDTransferRunnable.  At the end of that operation we fire the same event:

SYSTEM_REMOTE_RESOURCE_CREATED

Our views make use of these events to update the tree and table.  Given the time constraints of this release, I'm a little apprehensive of simply replacing these event firing with new events:  

SYSTEM_REMOTE_RESOURCE_MOVED
SYSTEM_REMOTE_RESOURCE_COPIED

Perhaps, we could initially fire the new events on top of what we already fire.

In that case, for move we'd have:

SYSTEM_REMOTE_RESOURCE_MOVED
SYSTEM_REMOTE_RESOURCE_DELETED
SYSTEM_REMOTE_RESOURCE_CREATED

In that case, for copy we'd have:

SYSTEM_REMOTE_RESOURCE_COPIED
SYSTEM_REMOTE_RESOURCE_CREATED
    
or we could do the following:

For move:

SYSTEM_REMOTE_RESOURCE_MOVED

For copy:

SYSTEM_REMOTE_RESOURCE_COPIED

However, there would be more work involved to change the event handling for these cases.

What do you think?

 
Comment 2 Martin Oberhuber CLA 2008-03-27 10:39:41 EDT
Could we just keep the existing events and add new fields to them:

SYSTEM_REMOTE_RESOURCE_DELETED
  --> Add "Moving to: absolute path" field
SYSTEM_REMOTE_RESOURCE_CREATED
  --> Add "Copied from: absolute path" field

When the new field is null, it's a plain creation / deletion without any move/copy operation.
Comment 3 David McKnight CLA 2008-03-27 12:47:22 EDT
Created attachment 93820 [details]
patch for describe remote events via operation field

Here is a patch that adds an optional operation field for remote events.

RSE provides the following operations out of the box:

	/**
	 * Indicates that the event is for a delete operation
	 */
	public static final String SYSTEM_REMOTE_OPERATION_DELETE = "DELETE"; //$NON-NLS-1$
	
	/**
	 * Indicates that the event is for a rename operation
	 */
	public static final String SYSTEM_REMOTE_OPERATION_RENAME = "RENAME"; //$NON-NLS-1$
	
	/**
	 * Indicates that the event is for a create operation
	 */
	public static final String SYSTEM_REMOTE_OPERATION_CREATE = "CREATE"; //$NON-NLS-1$
	
	/**
	 * Indicates that the event is for a move operation
	 */
	public static final String SYSTEM_REMOTE_OPERATION_MOVE   = "MOVE"; //$NON-NLS-1$
	
	/**
	 * Indicates that the event is for a copy operation
	 */
	public static final String SYSTEM_REMOTE_OPERATION_COPY   = "COPY"; //$NON-NLS-1$

However, users can define new ones since these are just strings.

To describe the old names, prior to a rename, move or copy operation, I've modified the oldName field and getOldName() method for the events to support multiples (since move and copy events often involve more than one src object).  As a result of these changes, I needed to update the events as well as the system registry methods that create them.

Let me know whether you think this approach is okay.
Comment 4 Martin Oberhuber CLA 2008-03-27 12:56:38 EDT
I can't really see what the "operation" should be good for. The kind of operation should be obvious from the other args. Can you come up with a case where you think the clients definitely need the "operation"?

What I'd like to see though is getting an indication about what subsystem the resources came from. It's possible to copy & paste across subsystems, e.g. DStore-files --> Local-files. The "oldAbsoluteNames[]" array just holds the absolute names which do not include that source subsystem as far as I know, right?

We do have some mangling code that creates a String to uniquely identify a remote object including its subsystem and absolute path. Perhaps this should be used here.
Comment 5 David McKnight CLA 2008-03-28 10:28:15 EDT
(In reply to comment #4)
> I can't really see what the "operation" should be good for. The kind of
> operation should be obvious from the other args. Can you come up with a case
> where you think the clients definitely need the "operation"?
> What I'd like to see though is getting an indication about what subsystem the
> resources came from. It's possible to copy & paste across subsystems, e.g.
> DStore-files --> Local-files. The "oldAbsoluteNames[]" array just holds the
> absolute names which do not include that source subsystem as far as I know,
> right?
> We do have some mangling code that creates a String to uniquely identify a
> remote object including its subsystem and absolute path. Perhaps this should be
> used here.

Strange, I thought I replied to this yesterday.  Anyway, an example scenario I have is this:

User does a move which results in first a DELETE event followed by a CREATE event.  A listener that receives the DELETE event needs to know the "operation" in order to tell this is the result of a move as opposed to a delete.  Even though the args of the old absolute names are there, that information still could be used in a delete operation so it's not enough just to have those args.

As for qualifying the names with the subsystem id, we could do that, as we have for drag and drop, although we need to make sure not the break the previous functionality.  Perhaps we could have an additional api that returns the names relative to the subsytem.
Comment 6 Martin Oberhuber CLA 2008-03-28 10:34:35 EDT
I thought that DELETE could have a new "targetPath" argument which would be empty in case of delete but existent in case of a MOVE.

Really, I can't find a scenario that requires the operation, an you?

You're right of course that adding the subsystem must not break existing clients. But any newly added fields (like targetPath / sourcePath) could be mangled names like it's done for drag&drop.
Comment 7 David McKnight CLA 2008-03-28 10:53:46 EDT
(In reply to comment #6)
> I thought that DELETE could have a new "targetPath" argument which would be
> empty in case of delete but existent in case of a MOVE.
> Really, I can't find a scenario that requires the operation, an you?
> You're right of course that adding the subsystem must not break existing
> clients. But any newly added fields (like targetPath / sourcePath) could be
> mangled names like it's done for drag&drop.

Even if the overall operation can be inferred from the arguments, I would find programmatic usage to be uncomfortable.  We'd be asking developers to try to guess what the actual operation is by looking at paths that we store in the event when instead we could be explicit about it. 
Comment 8 Martin Oberhuber CLA 2008-03-28 12:56:15 EDT
It looks like you committed the fix with a wrong checkin comment,
  [223204] [cleanup] fix broken nls strings in files.ui and others
Comment 9 David McKnight CLA 2008-03-28 13:05:48 EDT
(In reply to comment #8)
> It looks like you committed the fix with a wrong checkin comment,
>   [223204] [cleanup] fix broken nls strings in files.ui and others

I accidently committed SystemDNDTransferRunnable with that.  I've since taken that back.
Comment 10 Martin Oberhuber CLA 2008-03-28 14:24:00 EDT
Looks like you've committed more with this message:

SystemTableTreeView
SystemTableViewPart
SystemView
SystemScratchpadView
SystemBaseCopyAction
SystemTableView
Comment 11 David McKnight CLA 2008-03-28 14:34:55 EDT
(In reply to comment #10)
> Looks like you've committed more with this message:
> SystemTableTreeView
> SystemTableViewPart
> SystemView
> SystemScratchpadView
> SystemBaseCopyAction
> SystemTableView

Yeah, I realized that after.  I've committed the rest of the changes now too so it's all in cvs now. 
Comment 12 David McKnight CLA 2008-04-02 17:07:20 EDT
Marking this as fixed since the code was already committed.