Add image annotation via libKImageAnnotator
ClosedPublic

Authored by nicolasfella on Jun 24 2019, 3:35 PM.

Details

Summary

Adds an annotaion mode that makes use of https://github.com/ksnip/kImageAnnotator

While in annotation mode all other actions are disabled.
When the annotation mode is left the changes are applied

FEATURE: 372464
FIXED-IN: 19.08.0

Closes T6321

Test Plan

Diff Detail

Repository
R166 Spectacle
Branch
imageAnnotator
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13270
Build 13288: arc lint + arc unit
nicolasfella created this revision.Jun 24 2019, 3:35 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptJun 24 2019, 3:35 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
nicolasfella requested review of this revision.Jun 24 2019, 3:35 PM
nicolasfella edited the summary of this revision. (Show Details)Jun 24 2019, 3:38 PM
nicolasfella edited the test plan for this revision. (Show Details)
nicolasfella added a reviewer: Spectacle.
  • Fix build without libkimageannotator
nicolasfella edited the summary of this revision. (Show Details)Jun 24 2019, 3:51 PM
nicolasfella added a reviewer: dporobic.

This is really nice. I love that it's not even a huge amount of code either. Also +1 for making it conditional on finding the lib. Unfortunately, the lib is not yet distributed with KDE Frameworks, and it also depends on another widget that currently lives in a private github repo: https://github.com/DamirPorobic/kColorPicker. We'll need to upstream that so that libkimageannotator will even build cleanly, which seems important anyway considering that it's a KDE project.

Do you think we should wait until all the bits are in KDE first, or get this in sooner so that the feature magically works once the supporting libs are available?

cfeck added a subscriber: cfeck.Jun 24 2019, 6:55 PM

There is no requirement that dependencies need to be KDE projects, as long as they are optional (and maintained).

There is no requirement that dependencies need to be KDE projects, as long as they are optional (and maintained).

That's true, but I'm not sure that the external KColorPicker library is widely packaged given that it was created especially for KSnip. But maybe I'm wrong (it's just a guess after all). And I'm of course quite interested in getting this landed sooner rather than later. :)

BTW, the KDE Applications 19.08 dependency freeze is in 11 days: https://community.kde.org/Schedules/Applications/19.08_Release_Schedule#Thursday.2C_July_11.2C_2019:_KDE_Applications_19.08_Dependency_Freeze

So if we want to get this into 19.08.0, we'll need to figure out what we want to do about the dependencies before then. And if we plan to upstream KColorPicker into KWidgetsAddons, that needs to happen in time for KDE Frameworks 5.60, which is tagged in 6 days.

davidre added a subscriber: davidre.EditedJul 1 2019, 8:29 AM

Hmm I can only get libkimageannotator to build when kcolorpicker is installed to /usr/local not into my prefix. Otherwise cmake finds it but the headers aren't found during the build. Maybe we would have to invest time into bringing it's cmake files up to par?

Same when trying to build spectacle with libkimageannotator. I guess I'm doing something wrong?

libkimageannotator produces static libs by default. Maybe building shared libs helps? See https://github.com/DamirPorobic/kImageAnnotator#shared-vs-static

andisa added a subscriber: andisa.Dec 1 2019, 9:19 AM

It seems to me that KColorPicker has not been pushed into KWidgetsAddons until now. Maybe it would also help to port KImageAnnotator to make use of KColorButton instead which is already included in the widgets?
As I am no developer I am not sure if this could help but it seems to me that this dependency/build problem is the last piece left before it could be shipped, right?

Whats the process for adding new Widgets to KWidgetsAddons?

Whats the process for adding new Widgets to KWidgetsAddons?

Upload a patch here on Phabricator

dporobic added a comment.EditedJan 22 2020, 12:56 PM

All commits for kColorPicker:

Meanwhile there have been some updates to kImageAnnotator, maybe you want to add them too, even tough some unit tests are currently broken, I need to fix them, so potentially more commits coming.

All commits for kColorPicker:

Meanwhile there have been some updates to kImageAnnotator, maybe you want to add them too, even tough some unit tests are currently broken, I need to fix them, so potentially more commits coming.

Unit tests fixed.

@nicolasfella ping! Any chance of getting this in for 20.04?

ngraham accepted this revision.Apr 15 2020, 5:46 PM
This revision is now accepted and ready to land.Apr 15 2020, 5:46 PM
nicolasfella edited the summary of this revision. (Show Details)
  • Fix

If you had plans to to fix the libkimageannotator build failure, you might want to just commit it yourself. I don't think the author is paying attention to the version on KDE infrastructure as I see it's developed on GitHub again at https://github.com/ksnip/kImageAnnotator.

...For that matter maybe it makes sense to just link against that library rather than the old version on KDE infra.

The reason that the spectacle window is larger on start because the annotator cannot be smaller because of the annotation tools sidebar I think

The reason that the spectacle window is larger on start because the annotator cannot be smaller because of the annotation tools sidebar I think

Yes, the sidebar has a min width and height.

I know it's a bit ugly but maybe we can then dynamically construct/destroy the annotator on button press to combat this?

src/Gui/KSWidget.cpp
188

Duplicated in Line 62

Isn't it possible to the annotator? In ksnip it's hidden as long as you have no screenshot, so ksnip on startup is basically a toolbar.

Isn't it possible to the annotator? In ksnip it's hidden as long as you have no screenshot, so ksnip on startup is basically a toolbar.

I'm not an expert with layouts and sizehinhts and related things in widgets but I couldn't figure out how. The stack always seemed to take the annotators size in account.

It would be nicer to not disable all other actions when in annotator mode, but rather just save the image then continue with the action.

  • That's one step less for the users, and a lot of steps less if they are making a lot of screenshot and annotating them before saving or sharing.
  • It's the same behavior with many popular screenshot tools. They don't require user to turn off the annotation mode to be able to save/share the screenshot.

Since this change didn't make it to 20.04, We should have more time to put more effort in coding the above UX until the next Application release.

Would be really nice to finally get this in.

Can this be moved over to invent.kde.org and merged?

nicolasfella edited the summary of this revision. (Show Details)Sep 2 2020, 4:42 PM

Merging this as discussed in the VDG chat

nicolasfella closed this revision.Sep 2 2020, 4:47 PM
volkov added a subscriber: volkov.Dec 18 2020, 6:45 PM

Awesome feature!

Unfortunately it's missing translations' loading.
Here is one approach to this: https://github.com/ksnip/kImageAnnotator/pull/173