Bugzilla will undergo maintenance 2024-03-29 18h00 CET. Bugzilla will be placed in read-only mode at that time.

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 462623 - Allow to easily extend GdbAdapterFactory
Summary: Allow to easily extend GdbAdapterFactory
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 8.5   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 8.7.0   Edit
Assignee: Marc Khouzam CLA
QA Contact: Marc Khouzam CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-19 21:29 EDT by Marc Khouzam CLA
Modified: 2015-04-17 11:24 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Khouzam CLA 2015-03-19 21:29:20 EDT
The class GdbAdapterFactory creates all the different adapter of a DsfSession.  If extenders want to modify some of the adapters, they have to copy the entire class.

We should rework the class to be able to override it easily.
Comment 1 Eclipse Genie CLA 2015-03-19 21:31:17 EDT
New Gerrit change created: https://git.eclipse.org/r/44220
Comment 2 Marc Khouzam CLA 2015-03-19 21:40:52 EDT
(In reply to Eclipse Genie from comment #1)
> New Gerrit change created: https://git.eclipse.org/r/44220

This patch splits GdbAdapterFacory into two classes making it easy to override.

We can see an example on how to override it in the patch also, as part of o.e.cdt.examples.dsf.gdb

There are a couple of things I'm not happy with:

1- the set of adapters, now called GdbSessionAdapters, are no longer @Immutable.  I thing that is ok, but really, after construction, those adapters really should not change.  An alternative is not to make every field in GdbSessionAdapter protected (which I usually don't like anyway) and have a create<Field> protected method.  That way each field could be final and the class @Immutable.  The annoying part of this is that we'd need to create such a "create" method for every field.  Unless we only did it for ones that we think are worth overriding.

2- Everything in there is actually internal.  I'd like to start making some parts APIs, but I don't want to have to make every adapter API just yet.  If we only allow to override the adapter type we care about, we could make those APIs, and gradually address the others as the need arise.

I'll check with extenders on the list.
Comment 3 Nobody - feel free to take it CLA 2015-03-23 13:59:33 EDT
(In reply to Marc Khouzam from comment #2)
> (In reply to Eclipse Genie from comment #1)
> > New Gerrit change created: https://git.eclipse.org/r/44220
> 
> This patch splits GdbAdapterFacory into two classes making it easy to
> override.
> 
> We can see an example on how to override it in the patch also, as part of
> o.e.cdt.examples.dsf.gdb
> 
> There are a couple of things I'm not happy with:
> 
> 1- the set of adapters, now called GdbSessionAdapters, are no longer
> @Immutable.  I thing that is ok, but really, after construction, those
> adapters really should not change.  An alternative is not to make every
> field in GdbSessionAdapter protected (which I usually don't like anyway) and
> have a create<Field> protected method.  That way each field could be final
> and the class @Immutable.  The annoying part of this is that we'd need to
> create such a "create" method for every field.  Unless we only did it for
> ones that we think are worth overriding.
> 

I agree with you, using protected fields is not a good idea. At the moment I don't see anything better than using "create" methods.

> 2- Everything in there is actually internal.  I'd like to start making some
> parts APIs, but I don't want to have to make every adapter API just yet.  If
> we only allow to override the adapter type we care about, we could make
> those APIs, and gradually address the others as the need arise.
> 
> I'll check with extenders on the list.

I think it would be helpful to identify possible use cases and see how the proposed solution addresses them. For instance, overriding the VM provider for a view is the task that I have needed quite a few times. With the proposed patch it can only be achieved by duplicating all existing VM providers. Same applies to most of the command handlers. After all this duplications, duplicating GdbLaunch is not a big deal.
I guess, most of the clients would opt for using the corresponding internal classes but that the indication of problems with API.
Comment 4 Marc Khouzam CLA 2015-03-23 16:02:51 EDT
(In reply to Mikhail Khodjaiants from comment #3)

> I agree with you, using protected fields is not a good idea. At the moment I
> don't see anything better than using "create" methods.

We can start with the "create" methods for use-cases that seem valuable.

> I think it would be helpful to identify possible use cases and see how the
> proposed solution addresses them. For instance, overriding the VM provider
> for a view is the task that I have needed quite a few times. With the
> proposed patch it can only be achieved by duplicating all existing VM
> providers. 

Can't you override GdbViewModelAdapter and then only change the VMProvider you want to change?  That is what I've done in GdbExtendedViewModelAdapter and GdbExtendedLaunchVMProvider.  And with this patch, we no longer need to copy GdbAdapterFactory but we can extend it.

> Same applies to most of the command handlers. 

Could you provide an example using o.e.cdt.examples.dsf.gdb?

> After all this
> duplications, duplicating GdbLaunch is not a big deal.

I thought people could extend GdbLaunch?

> I guess, most of the clients would opt for using the corresponding internal
> classes but that the indication of problems with API.

Yeah, right now they probably do but I'd like to start making some of the classes that are actually extended, API.  But as you said, we should identify use-cases.  I was hoping you could provide input on what would help you; it would be a good starting point.
Comment 5 Nobody - feel free to take it CLA 2015-03-23 17:26:35 EDT
(In reply to Marc Khouzam from comment #4)
> (In reply to Mikhail Khodjaiants from comment #3)
> 
> > I agree with you, using protected fields is not a good idea. At the moment I
> > don't see anything better than using "create" methods.
> 
> We can start with the "create" methods for use-cases that seem valuable.
> 
> > I think it would be helpful to identify possible use cases and see how the
> > proposed solution addresses them. For instance, overriding the VM provider
> > for a view is the task that I have needed quite a few times. With the
> > proposed patch it can only be achieved by duplicating all existing VM
> > providers. 
> 
> Can't you override GdbViewModelAdapter and then only change the VMProvider
> you want to change?  That is what I've done in GdbExtendedViewModelAdapter
> and GdbExtendedLaunchVMProvider.  And with this patch, we no longer need to
> copy GdbAdapterFactory but we can extend it.
> 
> > Same applies to most of the command handlers. 
> 
> Could you provide an example using o.e.cdt.examples.dsf.gdb?
> 
> > After all this
> > duplications, duplicating GdbLaunch is not a big deal.
> 
> I thought people could extend GdbLaunch?
> 
> > I guess, most of the clients would opt for using the corresponding internal
> > classes but that the indication of problems with API.
> 
> Yeah, right now they probably do but I'd like to start making some of the
> classes that are actually extended, API.  But as you said, we should
> identify use-cases.  I was hoping you could provide input on what would help
> you; it would be a good starting point.

Sorry, ignore my comments about duplications. I guess, I have been doing too many things at the same time. 
Most of my experience with extending the DSF/GDB code comes from attempts to move some of our CDT changes to our plugins. In many cases it's doable even with the current implementation but may be tricky for new users, especially when modifying the UI parts. 
Regarding use cases. Should we consider providing examples for replacing an existing command handler or adding a new one?
Comment 6 Marc Khouzam CLA 2015-03-25 09:52:15 EDT
(In reply to Mikhail Khodjaiants from comment #5)
 
> Regarding use cases. Should we consider providing examples for replacing an
> existing command handler or adding a new one?

Absolutely.  I would like the o.e.cdt.examples.dsf.gdb to show how to extend as many different aspect of DSF-GDB.  And as we add certain such use cases, we will surely realize that we should make parts of the code easier to extend.

This is really what happened to me in this case here, where I wanted to change a lable for a VMNode and had to completely copy GdbAdatorFactory.  So I tried to make it easier to extend with this bug.

I still plan to post a improved patch following your comments about not using protected for every field.
Comment 7 Marc Khouzam CLA 2015-03-27 10:20:50 EDT
(In reply to Marc Khouzam from comment #6)

> I still plan to post a improved patch following your comments about not
> using protected for every field.

I posted an update that addresses issue 1 of comment 2, which was that GdbSessionAdapters was no @Immutable.  It is now @Immutable as before by using create() methods.  I've only added a couple of those.

I believe this patch brings no change in behaviour, but only allows to more easily extend the class.

As for making some of these APIs, I suggest we do this separately.
Comment 8 Nobody - feel free to take it CLA 2015-03-31 23:01:03 EDT
Marc, I have an idea on how to make GdbSessionAdapters extendible without adding "create" methods for each adapter. I'll try pushing my patch to Gerrit when it's ready unless you want me to post it here.
Comment 9 Marc Khouzam CLA 2015-04-01 13:50:47 EDT
(In reply to Mikhail Khodjaiants from comment #8)
> Marc, I have an idea on how to make GdbSessionAdapters extendible without
> adding "create" methods for each adapter. I'll try pushing my patch to
> Gerrit when it's ready unless you want me to post it here.

Gerrit is fine.  I'll mark the review as -1 waiting for your suggestion.
Comment 10 Nobody - feel free to take it CLA 2015-04-08 14:03:05 EDT
I've pushed a new patch set to Gerrit. The patch includes an example of a custom ITerminateHandler which extends DsfTerminateCommand.
Disposing the adapters looks a bit tricky but I couldn't come up with anything better.
Comment 11 Marc Khouzam CLA 2015-04-09 13:46:11 EDT
(In reply to Mikhail Khodjaiants from comment #10)
> I've pushed a new patch set to Gerrit. The patch includes an example of a
> custom ITerminateHandler which extends DsfTerminateCommand.
> Disposing the adapters looks a bit tricky but I couldn't come up with
> anything better.

I think your use of reflection is nicer than exhaustively calling each adapters dispose method before.  It is also more future-proof.  I like.
Comment 13 Marc Khouzam CLA 2015-04-17 11:24:56 EDT
Finally got this completed.

Thanks a lot Mikhail for the great help!