Make the item background color and page cache properties available from View component
Needs RevisionPublic
Actions

Authored by dcaliste on Sep 26 2018, 3:00 PM.

Details

Reviewers

leinir
danders
anthonyfieroni

Group Reviewers

Calligra: 3.0

Summary

Currently, the item background color and the page cache properties in Words are built-in values (_resp. _ set to QColor(232, 233, 234) and false). They cannot be changed from QML or at all (for the background color). Users of libCalligraComponents.so and friends must patch the sources to change their behaviours.

This PR introduces a method to set the background color in KWCanvasItem. It also expands Components::View by overloading the fillColor property of QQuickPaintedItem and adds a pageCacheEnabled(bool) property.

The two new properties are implemented for text documents and are irrelevant for the other kind of document (but may have some sense in the future, if needed).

Is it the right way to go?

Diff Detail

Repository

R8 Calligra

Lint

Lint Skipped

Unit

Unit Tests Skipped

dcaliste created this revision.Sep 26 2018, 3:00 PM

Restricted Application added a subscriber: Calligra-Devel-list. · View Herald TranscriptSep 26 2018, 3:00 PM

dcaliste requested review of this revision.Sep 26 2018, 3:00 PM

'm sorry but the background should not be settable - it's a document property at best and in fact paper is mostly white, so shouldn't even be settable

Sorry, I was not clear in my description, I'm not setting the paper background color, but the item background color, i.e. the small area around and between the pages. In most desktop application it is light grey. But when using the KWCanvasItem in another context, it would be nice to be able to set this color. It's kind of desktop color if you prefer.

Ahh well in that case I don't mind and the words code looks clean enough - I'll let someone else review the components

Looks pretty good to me. It'd be nice if we could avoid exposing DocumentImpl (it's supposed to be sort of hidden), if you could have a think on a way to avoid that, i'd appreciate it :)

components/View.cpp
139	i'm always very uncomfortable with introducing returns inside statements... i'd very much like if you could refactor this so it doesn't just return like this (perhaps merge the two if statements, something like `if (pageCache() != withCache && d->document && d->document->impl())`?). i know it's semantically identical, but maintainability of early returns is just... super awkward ;)
words/part/KWCanvasBase.h
106	It'd be nice to have docs on new public APIs, please :) i realise it's trivial and "just right there", but the api docs page doesn't show stuff quite so conveniently without a bit of help ;)

This revision now requires changes to proceed.Sep 28 2018, 10:54 AM

Thank you @leinir for your review. I've not disappeared, but I'm still thinking about a way not to expose the impl object from Document class, while not cluttering the Document API with methods like setBackgroundColor(), setPageCache() or any other future canvas methods that I may think about.

So still working on this and I'll come back with a solution when I'll have one!

Also can you add parentheses over single line code and if you know better way to get impl() without duplicate calls will be great :)

components/View.cpp
160	You don't check against impl is not nullptr?

danders added inline comments.Oct 4 2018, 12:31 PM

components/View.h
118	Could you rename pageCache() to pageCacheEnabled() and d:o setPageChache()

ping

Revision Contents
Changeset List

			Path	Packages
M			components/Document.h (1 line)
M			components/Document.cpp (11 lines)
M			components/View.h (28 lines)
M			components/View.cpp (34 lines)
M			components/impl/DocumentImpl.h (5 lines)
M			components/impl/DocumentImpl.cpp (13 lines)
M			components/impl/TextDocumentImpl.h (3 lines)
M			components/impl/TextDocumentImpl.cpp (20 lines)
M			words/part/KWCanvasBase.h (1 line)
M			words/part/KWCanvasBase.cpp (5 lines)
M			words/part/KWCanvasItem.h (7 lines)
M			words/part/KWCanvasItem.cpp (19 lines)