D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  All painting of documents is done in points
  
  the purpose of KoUnit is to allow the user to specify some size in any unit 
so that it internally can be converted to points.
  
  Pixels are very special in that they are not a unit at all, but for special 
cases like karbon and krita it makes sense to allow such specification anyway, 
since the output is a bitmap. where a pixel is a very real unit.
  
  The trouble with allowing pixels to be specified in the general sense is that 
the ppi is dependent of zoom and of display. This will vary from user to user 
and in the case of zoom even as the user zooms in and out.
  
  So yes please make a karbon only solution, but even in karbon you should 
allow the user to specify the target ppi. For Icons you may get away with an 
assumption of 96 ppi but even that is really wrong, it just happens to work as 
most have it wrong the same way.
  
  But such assumptions will break any kind of real workflow for the other 
applications so it is very important not to make such a mistake.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D15111#672155 , @boemann wrote:
  
  > Even for line thickness it doesn't make sense to enable it in general. the 
image will change if you zoom in and out.
  
  
  So it changes on any other type in same way, you know centimetres, for 
example, keep same but image.
  
  > But you do NOT do it in a general way for all apps.
  
  I can do it only for Karbon in config.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  Even for line thickness it doesn't make sense to enable it in general. the 
image will change if you zoom in and out.
  
  If you want to have line thickness expressed as pixels based on a destination 
rendering, then you make a separate dialog, where you (in this specific 
instance) allow the user to specify number of pixels. Then you convert the 
pixel thickness into a physical thickness based on the target rendering scale.
  
  If you do that then all other apps will still work, with no ill effects and 
karbon users can specify it in karbon.
  
  But you do NOT do it in a general way for all apps. Only karbon does 
rendering to a specific bitmap. All other apps are just showing a document at a 
zoom level - in this case you definitely don't what to have something display 
relative.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added a comment.


  I miss something here, the idea behind the patch is to have pixel metrics in 
thickness, geometry etc. @boemann i got your opinion as to have configurable 
pixel units, is that right?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Noah Davis]

ndavis added a comment.


  In D15111#671510 , @boemann wrote:
  
  > Nothing prevents Karbon from having a m_pixelUnit with ppi collected from a 
widget for user control
  >
  > and the use that unit like inkscape does.
  >
  > But what this patch does is enable it in general with an arbitrary chosen 
ppi value
  >
  > Why not measure in apples too - it will make as much sense ..
  
  
  I'm not an expert on this subject, so I'll just trust your opinion.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  Nothing prevents Karbon from having a m_pixelUnit with ppi collected from a 
widget for user control
  
  and the use that unit like inkscape does.
  
  But what this patch does is enable it in general with an arbitrary chosen ppi 
value
  
  Why not measure in apples too - it will make as much sense ..

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Noah Davis]

ndavis added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in KoUnit.h:59-60
> Are you using HiDPI resolution? Scale factor, Calligra is not hidpi 
> compatible, i think

I am using 96 font DPI (not forced) and 1x scaling at 1920x1080.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> ndavis wrote in KoUnit.h:59-60
> Was it already like that when I made the comment? When I export an SVG, it's 
> proportioned like the DPI was 72 if I open it in Gwenview or Inkscape. 48x48 
> becomes 36x36.

Are you using HiDPI resolution? Scale factor, Calligra is not hidpi compatible, 
i think

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Noah Davis]

ndavis added a comment.


  In D15111#671492 , @boemann wrote:
  
  > And that is why this proposal should be rejected as is
  >
  > pixels is not a unit unless assigned a ppi on a case by case basis
  >
  > and thus it needs to be something requested and not EVER something that is 
always available !
  
  
  Is px in Inkscape a different kind of unit from px in Karbon? Some kinds of 
vector graphics //have// to be done with pixel units, such as icons. It's fine 
if you think Karbon should focus primarily on making graphics for physical 
media, but you can't have a general purpose vector graphics program without 
supporting pixel units in the same way that Inkscape does.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  And that is why this proposal should be rejected as is
  
  pixels is not a unit unless assigned a ppi on a case by case basis
  
  and thus it needs to be something requested and not EVER something that is 
always available !

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Noah Davis]

ndavis added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in KoUnit.h:59-60
> @ndavis, DPI is 96, it's not configurable for now.

Was it already like that when I made the comment? When I export an SVG, it's 
proportioned like the DPI was 72 if I open it in Gwenview or Inkscape. 48x48 
becomes 36x36.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> KoUnit.h:59-60
>  #define CC_TO_POINT(cc) qreal((cc)*12.840103)
> +#define POINT_TO_PX(cc) qreal((cc)*96.0/72.0)
> +#define PX_TO_POINT(px) qreal((px)*72.0/96.0)
>  /**

@ndavis, DPI is 96, it's not configurable for now.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Noah Davis]

ndavis added a comment.


  This does need DPI configuration for pixel units to be useful. Or maybe you 
could just set the DPI to 96 for pixel units since that's probably what most 
people want.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 82815.
anthonyfieroni added reviewers: ndavis, VDG.
anthonyfieroni added a comment.


  Rebase on master

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15111?vs=41190=82815

REVISION DETAIL
  https://phabricator.kde.org/D15111

AFFECTED FILES
  filters/karbon/image/ImageExportOptionsWidget.cpp
  libs/main/KoView_p.h
  libs/main/config/KoConfigMiscPage.cpp
  libs/odf/KoUnit.cpp
  libs/odf/KoUnit.h
  libs/odf/tests/TestKoUnit.cpp
  libs/widgets/KoPageLayoutWidget.cpp
  sheets/part/Doc.cpp
  sheets/part/dialogs/PreferenceDialog.cpp

To: anthonyfieroni, boemann, danders, #calligra:_3.0, ndavis, #vdg
Cc: staniek, Calligra-Devel-list, davidllewellynjones, dcaliste, ognarb, 
cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Jarosław Staniek]

staniek added a comment.


  In D15111#317311 , @anthonyfieroni 
wrote:
  
  > @staniek did you want a DPI config spinbox?
  
  
  As in, e.g. GIMP, whenever we work in export-to-bitmap, and there's size 
other than in pixels, there's also DPI setting. That's all I can say 
design-wise.
  
  > Can you try this patch over master to approve it can be useful in Karbon? I 
keep in mind to make markers for SVG to have ability to save arrows, we have a 
bug report for that too. Maybe a little help will be needed :)
  
  I've commented on requirements only to confirm they exist and are not 
ephemeral. Sorry have not tested things.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: staniek, Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added a comment.


  @staniek did you want a DPI config spinbox? Can try this patch over master to 
approve it can be useful in Karbon? I keep in mind to make markers for SVG to 
have ability to save arrows, we have a bug report for that too. Maybe a little 
help will be needed :)

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: staniek, Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Jarosław Staniek]

staniek added a comment.


  In D15111#317263 , @boemann wrote:
  
  > we don't store pixel values but rather convert to points the user will not 
get pixel precise placement anyway
  
  
  Sure we but users who design for pixel size, like me, do that using grid 
alignment and this means whatever floating point offsets are used in memory or 
file they will be 100% precisely rounded to pixels upon exporting to the final 
raster file. That's how the use case works.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: staniek, Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  And none of that contradicts me saying it's not a general feature we want, 
but at most for Karbon, and that since we don't store pixel values but rather 
convert to points the user will not get pixel precise placement anyway

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: staniek, Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Jarosław Staniek]

staniek added a comment.


  tl;dr images designed without regards to physical pixels size look well only 
for large sizes
  
  Are we talking about equivalent of this option of Inkscape?
  
  F6223325: image.png 
  
  The Px unit has wide usage. In the bug description web developers are 
mentioned, natural part of the use cases include pixels measurement.
  
  My personal use case even for KEXI is to work with (Breeze) icons, as most of 
us know they are designed with pixels in mind, that is with 16x16, 22x22 etc. 
grids. Without this approach icons designed "for scalability" will not be 
visible at all *any* small size.
  
  Another point to vote yes for this change comes from the SVG specs. It's 
supported option. Not having it is a disservice for the user.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: staniek, Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  discard as a general solution - it doesn't make close to any sense for other 
than karbon when used as a tool to generate pixmaps in the end, and even then 
as I said it's a bad idea for various reasons

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added a comment.


  We'll rethink of being this option or i should discard as unwanted?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  The problem here is if they think they can place a line on a specific pixel 
position - we don't store coords as pixels - so  it will not end being rendered 
to a specific pixel anyway - we will just end up with users not getting what 
they think they get, even in such a case as you describe

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added a comment.


  I agree with that, but in some situations like targeting embedded device with 
especially knowing resolution :) I really don't know what can they'll see in 
pixels, but defined resolution is possible case.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  Yes, that is exactly how they could use it but, I think we are doing a 
disservice by offering such an option- there is no way an author can know the 
destination resolution and it will only apply to one destination.
  
  Besides why would they even need to know - we are not pixel base to begin 
with, we are all vectors
  
  I'm convinced that most users are just not understanding how wrong pixel unit 
is - we should not contribute by letting pixel units be available

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni added a comment.


  That's purely abstract, i think to add DPI spinbox next to pixel setting. I'm 
not a designer, but it looks like they want to work on *real* pixels that 
should look on the target device, that's maybe DPI config will be relevant.

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Camilla Boemann]

boemann added a comment.


  What is the purpose of showing pixel units in general - pixels is not really 
a unit except in very specific cases - I'd saythis is very very wrong
  
  What is the size of pixel in your mind anyway?

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 40536.
anthonyfieroni added a comment.


  Fix Tests as well.
  Notable change is that i made to always use round functions toXXX, do we need 
precision as offer qreal ?

REPOSITORY
  R8 Calligra

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15111?vs=40532=40536

REVISION DETAIL
  https://phabricator.kde.org/D15111

AFFECTED FILES
  libs/main/KoView_p.h
  libs/main/config/KoConfigMiscPage.cpp
  libs/odf/KoUnit.cpp
  libs/odf/KoUnit.h
  libs/odf/tests/TestKoUnit.cpp
  libs/widgets/KoPageLayoutWidget.cpp

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever

D15111: [KoUnit] Let's show pixel units

[Anthony Fieroni]

anthonyfieroni created this revision.
anthonyfieroni added reviewers: boemann, danders, Calligra: 3.0.
Herald added a project: Calligra: 3.0.
Herald added a subscriber: Calligra-Devel-list.
anthonyfieroni requested review of this revision.

REVISION SUMMARY
  CCBUG: 312739

REPOSITORY
  R8 Calligra

REVISION DETAIL
  https://phabricator.kde.org/D15111

AFFECTED FILES
  libs/main/KoView_p.h
  libs/main/config/KoConfigMiscPage.cpp
  libs/odf/KoUnit.cpp
  libs/odf/KoUnit.h
  libs/widgets/KoPageLayoutWidget.cpp

To: anthonyfieroni, boemann, danders, #calligra:_3.0
Cc: Calligra-Devel-list, cochise, vandenoever