Community
Participate
Working Groups
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.
New Gerrit change created: https://git.eclipse.org/r/44220
(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.
(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.
(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.
(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?
(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.
(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.
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.
(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.
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.
(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.
Gerrit change https://git.eclipse.org/r/44220 was merged to [master]. Commit: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=d071e969b73264b7b54242a1c38ee7786453c087
Finally got this completed. Thanks a lot Mikhail for the great help!