Re: Review Request: Default - empty char format is used for inline (as-char) shapes.

> On 2010-11-18 16:49:17, Casper Boemann wrote:
> > Obviously the argument of not using bool is valid, but in this case using a charformat, would mean many pleases where calling with the charformat of the cursor would be needed.
> > A bool that defaults to false means much fewer places that needs code update.
> > I'm just saying that coding for the future pays an impact in the present.
> >
> 
> Thomas Zander wrote:
>     >  using a charformat, would mean many pleases where calling with the charformat of the cursor would be needed.
>     
>     Can you explain why that is so?
>     Both a bool can have a default ( = false) and a QTextCharFormat can also have a default  (see QTextCursor API, insertBlock for instance). So there is not really any difference as far as I can see. Am I missing something?
>     
>     Unless I'm missing the point of this patch, the default empty charFormat is what you want to use in most usecases. The latest revision of the patch is missing usecases right now that could easily be fixed but are not with the approach Pavol choose. So I'm wondering why that he didn't follow my advice and went another way which introduces more bugs.
>     For instance if a user inserts an image in his current document then the exact same problem will appear and there may be underlines and all that stuff on his image.
>     
>     I suggested a solution that solves this in a previous thread that would solve both issues in one go. Is there any reason to not use that approach?
> 
> Casper Boemann wrote:
>     Well I suggested that he examine the bool road, because I was under the impression that most cases would require the textformat of the current cursor. Even though QTextCharFormat can have a default value that wouldn't help much if most cases would need a specific value. A specific value that could easily be computed inside the method. That is why I suggest to investigate the bool.
>     
>     However if indeed only a few usecases need a non-blank QTextCharFormat then the need for bool is not relevant.
>     
>     As to why he hasn't fixed all cases I'll let him answer himself, but I assume that this is just a technology demonstrator, getting the feedback he is getting now. So no need to implement all cases, before he is sure. But obviously all cases should be fixed.

about the bool; you have it reversed :)

KoInlineObject (the base class) needs a blank format. Rationale; we don't want underlines behind our images.
KoVariable (one subclass) needs the current format. Rationale; we want the text-based variable to keep the color and text-size.

AFAIK there is only one place where the KoVariable is inserted. Which would make it easier to change only that place.
On the other hand there can be many different types of inline-objects which all want a blank format. I would conclude we should probably optimize with the baseclass as the common case. Would you agree?

If you choose the opposite (like you suggested with the bool) you need to go through all of koffice and fix all places where this code is called. Which sounds like more work.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5898/#review8825
-----------------------------------------------------------


On 2010-11-18 11:21:01, Pavol Korinek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5898/
> -----------------------------------------------------------
> 
> (Updated 2010-11-18 11:21:01)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> insertInlineObject method in KoInlineTextObjectManager class has been changed. Inline object can be inserted using empty - default char format or current cursor char format. Empty - default char format is used for inline (as-char) shapes now.
> 
> 
> Diffs
> -----
> 
>   /trunk/koffice/libs/kotext/KoInlineTextObjectManager.h 1198325 
>   /trunk/koffice/libs/kotext/KoInlineTextObjectManager.cpp 1198325 
>   /trunk/koffice/libs/kotext/opendocument/KoTextLoader.cpp 1198325 
> 
> Diff: http://svn.reviewboard.kde.org/r/5898/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Pavol
> 
>

_______________________________________________
koffice-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/koffice-devel