Karbon: Enable multi page capability
AbandonedPublic

Authored by anthonyfieroni on Feb 22 2019, 10:00 AM.

Details

Reviewers
danders
Summary

Since odg spec supports multiple pages, I feel karbon also needs to support it.

Ported to use pageapp classes.

Karbon has config group "Interface" that is not precent in other apps.
Some of the options have been disabled atm. Imho they should be
harmonized with other apps and/or moved to View menu.

A lot of code was duplicated between pageapp and karbon
and has been removed from karbon:

  • Save/load
  • Layers docker and all layer operations
  • Grid, guides, rulers and zoom
  • Event handlers
  • Printing
  • Show page margins has been moved to pageapp

In general, import/export needs review to determine how to
handle multiple pages when e.g. exporting to a format that
does not support pages.

Known bugs:

  • "Separate paths" command:
    • Execute command, the shape disappears.
    • Undo crashes. Note: Afaics this code is not touched so probably a libs bug.
  • Snap to grid does not work
Test Plan

I am not an avid user of karbon, so would be nice if some that where could test.
Also, do not have all types of different format docs for filter testing.
Some that work are svg, jpg and karbon files.

Diff Detail

Repository
R8 Calligra
Branch
karbon_multipage_danders
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8672
Build 8690: arc lint + arc unit
danders created this revision.Feb 22 2019, 10:00 AM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptFeb 22 2019, 10:00 AM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
danders requested review of this revision.Feb 22 2019, 10:00 AM
rjvbb added a subscriber: rjvbb.Feb 22 2019, 10:57 AM

This would indeed be great to have; even a page selector when importing a multi-page document would be an improvement (the Adobe Illustrator version I've use had that; IIRC it would just leave all other pages of the document alone).

You should also test with PDF documents; in my experience Karbon 3.1 works well enough with them.

Ported to use pageapp classes.

...

A lot of code was duplicated between pageapp and karbon
and has been removed from karbon:

Shouldn't that be a separate change - or does multi-page support come automatically with this change?

This would indeed be great to have; even a page selector when importing a multi-page document would be an improvement (the Adobe Illustrator version I've use had that; IIRC it would just leave all other pages of the document alone).

You should also test with PDF documents; in my experience Karbon 3.1 works well enough with them.

Yes, pdf docs work, it is almost 100% handled by poppler.

...

A lot of code was duplicated between pageapp and karbon
and has been removed from karbon:

Shouldn't that be a separate change - or does multi-page support come automatically with this change?

Well, my initial thought, but there was going to be quite a few intermidiate solutions mainly because
the KoPACanvas cannot be subclassed, so I decided to do everything in one go.

anthonyfieroni added inline comments.Feb 22 2019, 1:46 PM
karbon/ui/widgets/KarbonConfigInterfacePage.cpp
73–90

I want to discuss comments in the review, i don't want to have in. why we remove this?

danders added inline comments.Feb 22 2019, 2:18 PM
karbon/ui/widgets/KarbonConfigInterfacePage.cpp
73–90

Recent file & docker font:
Why does karbon need these, none of the other apps have it.
Personally I would remove it, alternativly implent for all apps.
Canvas color:
I don't quite see what it is for. You can set a background color for the canvas but it is only for the views, it is not printed.
Also, if you have multiple views, it sets it in all views.

anthonyfieroni added inline comments.Feb 22 2019, 2:53 PM
karbon/ui/KarbonDocument.h
100–109

If they not used i.e. data is not used at all we can remove them, since they never have reason to be used we can safety remove.

karbon/ui/KarbonView.cpp
886–887

Why we comment, below as well?

1100

Why we check for the main window?

1102

You can not check it, i think. Group will be empty if not present then readEntry fill return false.

karbon/ui/widgets/KarbonConfigInterfacePage.cpp
73–90

We can remove "number of recent files" since other apps does not have it. but for other 2 i don't see why we remove them, someone can found useful (somehow). About me, i want them back, recent file complete remove not commented.

rjvbb added a comment.Feb 22 2019, 3:30 PM

Canvas color:
I don't quite see what it is for. You can set a background color for the canvas but it is only for the views, it is not printed.

A custom canvas colour feature doesn't strike me as odd, nor that it isn't printed (printing it WITHOUT setting a dedicated option would seem wrong to me).

What about when you use an inversed theme, isn't a custom canvas colour required then if you want to see your black line art on a light (white) canvas?

Canvas color:
I don't quite see what it is for. You can set a background color for the canvas but it is only for the views, it is not printed.

A custom canvas colour feature doesn't strike me as odd, nor that it isn't printed (printing it WITHOUT setting a dedicated option would seem wrong to me).

What about when you use an inversed theme, isn't a custom canvas colour required then if you want to see your black line art on a light (white) canvas?

No, the canvas is part of the document and must never be themed. The canvas background is as much part of your drawing as any line you put on it.

danders added inline comments.Feb 25 2019, 11:41 AM
karbon/ui/widgets/KarbonConfigInterfacePage.cpp
73–90

I reconsiddered this, sheets actually also implements this so I'll leave it in.
(It does not work properly neither for karbon nor sheets, but that for different rainy day)

73–90

I also reconsidder docker font setting. It is useful and (partially) implemented in libs. You have to restart to get the new setting.
This should be impleneted for all apps, but that's for a different patch.

But, when it comes to canvas background, the more a look into it the more I dislike it.
Afaics it goes against both odf spec, wysiwyg and pageapp implementation.
Unfortunatly it was implemented so long ago that there is not mail archive any longer and
the git log doesn't give any reason.
Unless I get a *very* good reason to implement this I will not include it.

totally agree about not theme'ing canvas

Also a general agreement to do the page app thing as long as it's also supported in svg

odg is hardly that much of a reason - it even seems like odf is moving away from odg as much as possible

danders marked an inline comment as done.Feb 25 2019, 12:14 PM
rjvbb added a comment.Feb 28 2019, 8:47 AM
No, the canvas is part of the document and must never be themed. The canvas background is as much part of your drawing as any line you put on it.

This hasn't been sitting right with me and I finally realised why.

That statement is true when you work with a real canvas and draw/paint/whatever directly onto that. If you take a grid paper the grid will become part of your art, but does Karbon print the grid which you can have it show (except possibly via an option)?

In drawing applications, the canvas is NOT part of your art, but simulates the canvas you'll be printing on. It's a backdrop layer that sits behind/below the lowest layer you can draw on. If you plan to print on a coloured piece of paper, or one with a canvassy texture, you'll probably want to adapt your virtual canvas so you can incorporate the physical canvas properties into your design. But you don't want that virtual canvas to print as that would be a waste of toner at best...

That's not to say that the virtual canvas should not be exportable at all: you'd want to be able to include it when generating a PDF (SVG, JPG, etc) format version for use in on-screen only presentations. Of course you could just add a backdrop layer in that case.

I'm not aware that Karbon has the equivalent of a slide/page design feature where you can define how each page will look. That could look be used to be achieve what I describe here but in Karbon that layer should probably not be printed by default (just like it isn't in presentation apps, IIRC).

No, the canvas is part of the document and must never be themed. The canvas background is as much part of your drawing as any line you put on it.

This hasn't been sitting right with me and I finally realised why.

That statement is true when you work with a real canvas and draw/paint/whatever directly onto that. If you take a grid paper the grid will become part of your art, but does Karbon print the grid which you can have it show (except possibly via an option)?

In drawing applications, the canvas is NOT part of your art, but simulates the canvas you'll be printing on. It's a backdrop layer that sits behind/below the lowest layer you can draw on. If you plan to print on a coloured piece of paper, or one with a canvassy texture, you'll probably want to adapt your virtual canvas so you can incorporate the physical canvas properties into your design. But you don't want that virtual canvas to print as that would be a waste of toner at best...

That is actual a valid point, although in say Krita with transparent pixels a checkerboard is shown.
That said it might make more sense for this "testing background" to be part of the document and not a general setting - if the use case is as you say then the testing background would also be different for different projects

That's not to say that the virtual canvas should not be exportable at all: you'd want to be able to include it when generating a PDF (SVG, JPG, etc) format version for use in on-screen only presentations. Of course you could just add a backdrop layer in that case.

I'm not aware that Karbon has the equivalent of a slide/page design feature where you can define how each page will look. That could look be used to be achieve what I describe here but in Karbon that layer should probably not be printed by default (just like it isn't in presentation apps, IIRC).

That is actual a valid point, although in say Krita with transparent pixels a checkerboard is shown.

Heh, I've had the opportunity to work closely with professional infographists so I picked up a thing or two about using applications like Illustrator and InDesign :)

How to indicate a transparent area of an object is yet another subject, with a related but not identical purpose.

That said it might make more sense for this "testing background" to be part of the document and not a general setting - if the use case is as you say then the testing background would also be different for different projects

True. I was reacting to *re*moving a setting completely, not to moving it to a more appropriate context.

That is actual a valid point, although in say Krita with transparent pixels a checkerboard is shown.

Heh, I've had the opportunity to work closely with professional infographists so I picked up a thing or two about using applications like Illustrator and InDesign :)

How to indicate a transparent area of an object is yet another subject, with a related but not identical purpose.

That said it might make more sense for this "testing background" to be part of the document and not a general setting - if the use case is as you say then the testing background would also be different for different projects

True. I was reacting to *re*moving a setting completely, not to moving it to a more appropriate context.

Ok, after another rethink...
It starts to make sence if karbon is used not as an odg editor but as an svg editor.
Let's say you create an icon that should be shown on certain background color(s), it would be a help if you could check your design aginst those colors.
In his case you are not concerned with wysiwyg or printing as you would be with an odg document.

anthonyfieroni commandeered this revision.Jun 4 2019, 8:34 PM
anthonyfieroni abandoned this revision.
anthonyfieroni edited reviewers, added: danders; removed: anthonyfieroni.

In flavor of D20400