[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ktexteditor-devel
Subject:    Re: KTextEditor container extension
From:       Philippe Fremy <phil () freehackers ! org>
Date:       2007-08-01 20:36:54
Message-ID: 46B0EEE6.2020809 () freehackers ! org
[Download RAW message or body]

Dominik Haumann wrote:
> On Wednesday 01 August 2007, Philippe Fremy wrote:
>> 	Hi,
>>
>> Thanks for the feedback so far.
>>
>> So, I returned to the simple API that was proposed initially. If a kpart
>> container provides the extension, he should support all the methods.
>>
>> All methods of ContainerExtension are pure virtual, except for the
>> constructor and destructor. I removed the d pointer of
>> ContainerExtension because of this: because the class is only an
>> interface, it won't store anything.
>>
>> I added a method to fetch the current activeView, as per Dominik
>> suggestion.
>>
>> Dominik Haumann wrote:
>>> class ...EXPORT... ContainerInterface {
>>>   virtual void setContainer( ContainerExtension * ext ) = 0;
>>>   virtual ContainerExtension * container() = 0;
>>> };
>>>
>>> Then, the following code peaces:
>>> ContainerInterface *iface = qobject_cast<ContainerInterface*>(editor);
>>> // cast the editor if (iface) {
>>>   // support + 100% implementation of ContainerExtension exists
>>> } else {
>>>   // not supported, now you can't do anything useful with
>>> ContainerExtension // anyway, so it's not supported
>>> }
>> I don't think it is possible this way. The kpart provides the editor,
>> but it is the kpart host that should provide a container extension.
> 
> Yes, above, I'm talking about an additional ContainerInterface to solve this,
> so this should still work:
> 
> Editor * editor = KTextEditor::EditorChooser::editor();
> ContainerInterface *iface = qobject_cast<ContainerInterface *>(editor);
>          ^^^^^^^^^
>          // not the INTERFACE, not ContainerExtension
> if (iface) {
> // ContainerExtension is supported
> ContainerExtension *ext = new ContainerExtension();
> iface->setContainer(ext);
> //...
> ContainerExtension *foo = iface->containerExtension();
> }

So, your code means that if the kpart host supports additional
capabilities, it should first check by casting editor to
ContainerInterface if those additional capabilities are actually needed
by the kpart, and then only set them.

I find this a bit complicated.

The way I see it: if the kpart host supports additional stuff, it sets
it with editor->setContainerExtension(). Many types of extensions can be
set, it is up to the kpart to check if they are supported and use them
if it wishes to.

> Something else: as Christoph pointed out, what about renaming
>   KTextEditor::ContainerExtension to
>   KTextEditor::Container ?
> 
> Of course it is an extension, but that is implicitly inherited because of
> the casting Editor* -> ContainerInterface*.

I prefer the API I proposed but if you think that this will be clearer
to understand for developers, I am ok to change it to whatever name you
want.

> 
>> The 
>> kpart host is not able to change the editor implementation. So, we need
>> setters and getters.
> 
> I still seem to miss something:
> 
> + * To check if this extension is supported, kpart code should do:
> + * \code
> + * Editor * editor = KTextEditor::EditorChooser::editor();
> + * ContainterExtension * ext = qobject_cast<ContainerExtension *>( editor->containerExtension() );
> + * if (ext != NULL ) {
> + *    doc = ext->createDoc();
> + *    // do something with the new doc
> + * } else {
> + *     // kpart can not request additional document
> + * }
> + * \endcode
> + */
> 
> Editor::containerExtension() returns a ContainerExtension *
> 1. why do I have to cast it with qobject_cast to ContainerExtension *?

My initial idea was to have a generic extension type that does nothing
but just mark the fact that it is an extension interface (see the very
first patch). And then propose different extensions, including a
MultiDocMultiView that does what the current ContainerExtension does:

class ExtensionInterface;

class MultiDocMultiViewExtension : public ExtensionInterface, public QObject
{
   Q_OBJECT
   /* create doc, remove doc, create view, remove view, change current
view, get current view */
}

class SomeOtherExtension : public ExtensionInterface, public QObject
{
   Q_OBJECT
	/* Some other stuff */
}

When we have that, it makes sense to cast to a given extension to check
if that extension is supported.

Christoph suggested to remove that scheme and to go for a single extension.

> 2. qobject_cast only works for QObject-derived classes, which is not the
>    case for ContainerExtension. (Derived classes need to contain then
>    Q_INTERFACES(ContainerExtension), but that won't work in this case).

You mean that it won't work even if the derived class inherits QObject ?

I'll check again what Q_INTERFACES() is for, but it seemed to me from my
initial reading that this was not needed.

> 
> How is ContainerExtension meant to work? Should it be casted to other classes
> in the future for more extensions?

Exactly.

> In other words: What about using ContainerInterface + Container, as
> mentioned above? ;)

I am all for having a generate ContainerInterface and specific container
extensions that inherit from ContainerInterface.

I find it just a bit complicated to go through a cast of Editor() to
request them where a simple setter/getter could do the trick.

> Yes, rename
> createDoc -> createDocument
> removeDoc -> removeDocument

Oops, I let that one slip.

	Philippe

_______________________________________________
KTextEditor-Devel mailing list
KTextEditor-Devel@kde.org
https://mail.kde.org/mailman/listinfo/ktexteditor-devel
[prev in list] [next in list] [prev in thread] [next in thread]