Bug 353351

Comment 1 Patrick Chuong 2011-12-06 17:33:55 EST

Created attachment 208030 [details]
Log error message instead of dialog

Comment 2 Anton Leherbauer 2011-12-07 02:51:11 EST

Simply logging is hardly visible to most users.  Maybe this could be made dependent on the error code of the status.  E.g. if the reason indicates that target is currently not available (IDsfStatusConstants.INVALID_STATE), log the error, otherwise put up a dialog.  What do you think?

Comment 3 Patrick Chuong 2011-12-07 11:41:28 EST

This should work. I'll adjust the patch as what you have suggested and commit it to HEAD.

Comment 4 Patrick Chuong 2011-12-07 15:07:29 EST

After thinking about this a little bit more, I am not sure whether this is appropriate to do. What if other client that also relies on the error code to do something differently? Then the service should to properly document what the return code should be, Pawel might be able to provide some input on this topic. Releying on the error code without proper documentation is more of a hack without proper documentation.

What if we change it at product customization? i.e add a preference to customize.ini file, or create an adapter for backend to override the default popup behavior?

Comment 5 Patrick Chuong 2011-12-08 14:32:18 EST

Created attachment 208113 [details]
preference

Toni, this patch is based on preference value to suppress the dialog, all errors go into the workspace log file if the preference value is set to true. By default, it is set to false.

Can you take a look when you get a chance? Thanks.

Comment 6 Anton Leherbauer 2011-12-09 04:02:03 EST

If you want to go this way, please
- put the constant into DisassemblyPreferenceConstants
- refactor the error reporting code into a protected method

Would it also be an option for you to use a derived version of DisassemblyBackendDsf overriding the error reporting method (instead of the preference)?

Comment 7 Patrick Chuong 2011-12-12 14:10:45 EST

Created attachment 208277 [details]
Using adapter to override error reporting

This patch uses an adapter to override the standard DisassemblyBackendDsf implementation. I like this approach over the preference approach, error is handle differently based on the active debugger.

To use this patch, one have to provide an adapter factory and the adapter factor should return the new instance of the backend when asked.

Let me know if there is any further adjustment that is required.

Comment 8 Anton Leherbauer 2011-12-14 03:13:30 EST

(In reply to comment #7)
> To use this patch, one have to provide an adapter factory and the adapter
> factor should return the new instance of the backend when asked.

Nice backdoor ;-)
I believe you can achieve a similar effect by registering your custom adapter factory for a more concrete type than IDMVMContext for which the default factory is registered.  But I am OK with this solution, too.

> Let me know if there is any further adjustment that is required.

I would ask you to make some cosmetic changes:
- rename method "errorHandler" to "handleError"
- remove arguments shell, title and msg
- move the method to AbstractDisassemblyBackend such that this becomes available 
  to non-DSF clients as well.
- include the fCallback.asyncExec() in the default implementation
  (logging does not need to be on the UI thread)

Comment 9 CDT Genie 2011-12-14 16:23:04 EST

*** cdt git genie on behalf of Patrick Chuong ***

    Bug 353351 - DisassemblyBackendDsf.retrieveFrameAddressInSessionThread()
    shows an error message to the user if stack.getFrameData fails.

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=cffc23093a9a22dc9a4b5e1a32a70772eccc37b7

Comment 10 Patrick Chuong 2011-12-14 16:40:10 EST

Created attachment 208409 [details]
committed patch

Thanks for the suggestions, I have committed the new patch to master.

Comment 11 Patrick Chuong 2011-12-14 16:42:35 EST

Resolved.