Bug 357424

Comment 1 Steven Hung 2011-09-13 15:51:06 EDT

Created attachment 203295 [details]
Patch v1.0

Overview of changes:
org.eclipse.wst.server.core.ServerEvent
New APIs for handling IStatus as well as a new STATUS_CHANGE constant

org.eclipse.wst.server.core.internal.Server
Introduce fireServerStatusChangeEvent and fireModuleStatusChangeEvent, called in setServerStatus and setModuleStatus methods respectively

org.eclipse.wst.server.ui.internal.cnf.ServersView2.refreshServerContent(IServer)
Deal with new events from statuses that would require a refresh of the server content or server state

Testing done:
All tests were done with a server adapter that will output state and status for the server and modules.

1. Server status change - ensure fireServerStatusChangeEvent method called in Server.java and refreshServerState method called in ServersView2 (to refresh the view)
2. Module status change - ensure fireModuleStatusChangeEvent method called in Server.java and refreshServerContent method called in ServersView2 (to refresh the view)
3. Tested existing server state and module state updates - ensure the view still updates correctly during server operations (start, restart, stop) and module actions (publish, start, stop, restart)

Comment 2 Elson Yuen 2011-09-14 12:08:35 EDT

A couple of comments:
1. How does the status check event pass the org.eclipse.wst.server.core.ServerEvent.checkKind() at the end of the constructor of org.eclipse.wst.server.core.ServerEvent.ServerEvent(int, IServer, IModule[], int, int, boolean, IStatus). Does checkKind() need to be modified to accept STATUS_CHANGE as well?  
2. Update copyright statements as needed.

Other than that, the logic looks good and testing is sufficient.

Comment 3 Elson Yuen 2011-09-14 12:10:25 EDT

Also, I recommend to do a regression test to make sure the existing server change event will cause the server view to refresh.

Comment 4 Steven Hung 2011-09-14 15:12:54 EDT

Created attachment 203362 [details]
Patch v1.1

Elson, regarding your comments:

1. The reason why org.eclipse.wst.server.core.ServerEvent.checkKind() had worked without adding the new STATUS_CHANGE was due to the existing logic in checkKind().

It checks for whether if (i != 0 && i != 1) or will return an IllegalArgumentException. In the cases where STATUS_CHANGE was used, it would return 0.

I have added STATUS_CHANGE to this method, as something that will increment the "i" value. There are 10 places where a ServerEvent is created, that would create different "kind" values. The output below shows the "kind" values and what the resulting "i" value would be:

kind=34 i=1
kind=36 i=1
kind=33 i=1
kind=40 i=1
kind=18 i=1
kind=34 i=1
kind=20 i=1
kind=16 i=0
kind=17 i=1
kind=24 i=1

The output ensures that the new increment will not cause an IllegalArgumentException for any of the existing ServerEvent creations (all are in the Server's fire____ methods).

The reason why i=0 is valid is due to the method fireServerChangeEvent, which will pass in the value ServerEvent.SERVER_CHANGE as the kind (no bitwise OR modifies its value)

2. The copyright headers were already updated to 2011, these files were previously modified in 2011

3. I have done regression testing to ensure that existing events called refreshServerState and refreshServerState (i.e. ServerEvents that were not STATUS_CHANGE, such as server and module state changes)

Comment 5 Steven Hung 2011-09-14 15:15:04 EDT

(In reply to comment #4)
> Created attachment 203362 [details]
> Patch v1.1
> 
> Elson, regarding your comments:
> 
> 1. The reason why org.eclipse.wst.server.core.ServerEvent.checkKind() had
> worked without adding the new STATUS_CHANGE was due to the existing logic in
> checkKind().
> 
> It checks for whether if (i != 0 && i != 1) or will return an
> IllegalArgumentException. In the cases where STATUS_CHANGE was used, it would
> return 0.
> 
> I have added STATUS_CHANGE to this method, as something that will increment the
> "i" value. There are 10 places where a ServerEvent is created, that would
> create different "kind" values. The output below shows the "kind" values and
> what the resulting "i" value would be:
> 
> kind=34 i=1
> kind=36 i=1
> kind=33 i=1
> kind=40 i=1
> kind=18 i=1
> kind=34 i=1
> kind=20 i=1
> kind=16 i=0
> kind=17 i=1
> kind=24 i=1
> 
> The output ensures that the new increment will not cause an
> IllegalArgumentException for any of the existing ServerEvent creations (all are
> in the Server's fire____ methods).
> 
> The reason why i=0 is valid is due to the method fireServerChangeEvent, which
> will pass in the value ServerEvent.SERVER_CHANGE as the kind (no bitwise OR
> modifies its value)
> 
> 2. The copyright headers were already updated to 2011, these files were
> previously modified in 2011
> 
> 3. I have done regression testing to ensure that existing events called
> refreshServerState and refreshServerState (i.e. ServerEvents that were not
> STATUS_CHANGE, such as server and module state changes)

I forgot to mention, I had done the same tests as mentioned in comment #1 and also additional breakpoint tests as described in point #3 in comment #4

Comment 6 Elson Yuen 2011-09-14 16:12:49 EDT

I updated the copyright statement for ServerEvent on 32M code stream.

Comment 7 Elson Yuen 2011-09-14 16:13:06 EDT

Code released to 32M and HEAD

Comment 8 Elson Yuen 2011-10-05 15:11:35 EDT

Code released to 33M

Comment 9 Eclipse Genie 2017-10-11 16:37:07 EDT

New Gerrit change created: https://git.eclipse.org/r/109070