Port QML Rectangle cropper to QWidget + QPainter
ClosedPublic

Authored by abalaji on May 1 2018, 3:17 AM.

Details

Summary
  1. Port QML canvas based screenshot cropper to QWidget + QPainter based cropper
  2. Fix minor bugs with cropper while porting, like system font not being set for size indicator
  3. "Include mouse pointer" setting now blends the cursor into the screenshot right away, letting the user see it while cropping, rather than blending it in after cropping

Before:


After:

BUG: 394503
BUG: 374009

Test Plan

Compare with original cropper and check if I missed something

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

This doesn't compile for me. Here's the build log: https://paste.kde.org/pvnqqtak1

This revision now requires changes to proceed.Nov 28 2018, 10:51 PM

Sorry @ngraham my bad, accidentally left some things uncommitted. It should now compile.

There we go, much better. :)

ngraham requested changes to this revision.Nov 29 2018, 12:06 AM

Awesome work, I'm impressed. Now it's really fast, much faster than the current implementation. I left a few inline comments.

As-is, this almost entirely fixes https://bugs.kde.org/show_bug.cgi?id=374009, so it's at least worth a CCBUG: 374009. I left an inline comment detailing how you can make it fix that bug entirely. :)

src/QuickEditor/QuickEditor.cpp
83

All of this text needs to be translated; use i18n() like Kai indicated.

90

This is missing everything about the arrow keys.

99

If we can make the full-screen overlay window not always stay on top (i.e. you can alt-tab out of it or switch virtual desktops while it's open) then that would probably be sufficient to fully fix https://bugs.kde.org/show_bug.cgi?id=374009 in conjunction with existing improvements that this patch provides.

110–121

Don't hardcode font sizes

This revision now requires changes to proceed.Nov 29 2018, 12:06 AM

Awesome work, I'm impressed. Now it's really fast, much faster than the current implementation. I left a few inline comments.

As-is, this almost entirely fixes https://bugs.kde.org/show_bug.cgi?id=374009, so it's at least worth a CCBUG: 374009. I left an inline comment detailing how you can make it fix that bug entirely. :)

Oh I see, I ran into that yesterday and had to switch to a TTY to kill the process. I'll look into fixing it entirely

abalaji updated this revision to Diff 46440.Nov 29 2018, 12:38 AM
  • Rename variable
  • Reorder includes
  • Don't stay on top
  • Don't hardcode font size
  • Use KLocalizedString
abalaji marked 4 inline comments as done.Nov 29 2018, 12:39 AM
abalaji added inline comments.
src/QuickEditor/QuickEditor.cpp
90

Sure, on it

abalaji updated this revision to Diff 46441.Nov 29 2018, 1:45 AM
abalaji marked 2 inline comments as done.
  • Update help text
abalaji updated this revision to Diff 46442.Nov 29 2018, 2:05 AM
  • Microoptimize some arithmetic

@ngraham @broulik I think I addressed all the comments. Let me know if there's anything else

Wonderful. I can confirm that this fully fixes https://bugs.kde.org/show_bug.cgi?id=374009, so go ahead and add BUG: 374009 to the summary section.

Just a few more code change requests from me, and I'll let @broulik do the code review heavy lifting! :)

src/QuickEditor/QuickEditor.cpp
72

There's a special way we handle markup and line breaks in strings; change i18n("some\ntext") to xi18nc("@info", "some<nl/>text")

For more info, see https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

78

What are all these &nbsp;s for? This seems like the wrong way to do whatever you're trying to do here.

abalaji edited the summary of this revision. (Show Details)Nov 29 2018, 9:22 AM
abalaji added inline comments.Nov 29 2018, 9:30 AM
src/QuickEditor/QuickEditor.cpp
78

I'm using QStaticText for performance so the text layout information is not recalculated with each paint. As it turns out, the only way to easily accomplish linebreaks with QStaticText is using rich-text and <br>, but QStaticText with rich-text has a weird issue where it considers the width of everything before the first <br> (the first line) to be the maximum width for the entire piece of text, and thus wraps the text on each subsequent line to fit this width. I'm using non-breaking spaces to force it to put them on a single line. Try getting rid of all the &nbsp;'s and you'll see what I mean. I'll try and figure out a different solution for this.

You removed setWindowFlags(Qt::BypassWindowManagerHint | Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::Tool) to fix https://bugs.kde.org/show_bug.cgi?id=374009, but this broke QuickEditor on multiscreen cofiguration.

Unfortunatelly because of showFullScreen() the widget size is set by window manager to the size of first screen.

If you set Qt::BypassWindowManagerHint it works ok on multiscreen config, but you have bug 374009.

You removed setWindowFlags(Qt::BypassWindowManagerHint | Qt::WindowStaysOnTopHint | Qt::FramelessWindowHint | Qt::Tool) to fix https://bugs.kde.org/show_bug.cgi?id=374009, but this broke QuickEditor on multiscreen cofiguration.

Unfortunatelly because of showFullScreen() the widget size is set by window manager to the size of first screen.

If you set Qt::BypassWindowManagerHint it works ok on multiscreen config, but you have bug 374009.

Darn, I guess we need to revert this change and try to fix 374009 another way. Any suggestions?

Well, Qt::BypassWindowManagerHint is not really an option since the window manager is what's responsible for Alt-Tab and things like that. We'll have to make the widget extend across multiple screens some other way.

I was experimenting some with that and finnaly I've found solution!

Replace showFullScreen(); with

setWindowFlags(Qt::Tool|Qt::FramelessWindowHint|Qt::NoDropShadowWindowHint|Qt::WindowStaysOnTopHint);
show();

I was experimenting some with that and finnaly I've found solution!

Replace showFullScreen(); with

setWindowFlags(Qt::Tool|Qt::FramelessWindowHint|Qt::NoDropShadowWindowHint|Qt::WindowStaysOnTopHint);
show();

Awesome! Works perfect!

abalaji updated this revision to Diff 46507.Nov 29 2018, 9:01 PM
  • Make bottom text static
  • Fix multi-screen situation

Without Qt::WindowStaysOnTopHint I see panel above QuickEditor :)

Well, with WindowStaysOnTop, what about 374009?

kpiwowarski added a comment.EditedNov 29 2018, 9:13 PM

I think it's fixed. Yes, QuickEditor is always on top but after clicking anywhere you have keyboard focus again :)

kpiwowarski added a comment.EditedNov 29 2018, 9:16 PM

But... there is another solution - you can replace Qt::Tool with Qt::Popup and there will be no possibility to use Alt+Tab until you exit selection mode. So, you never lost keyboard focus, I think that's the best solution to use Qt::Popup.

Hmph it was working properly with multiple screens, but with a single screen, it dodges the panels for me regardless of Qt::WindowStayOnTopHint. There should be a way to tell Qt to not do this.

No nevermind that did the trick, Qt::Popup is the way to go πŸ‘ thanks @kpiwowarski!

abalaji updated this revision to Diff 46510.Nov 29 2018, 9:27 PM
  • Fix window flags
abalaji updated this revision to Diff 46512.Nov 29 2018, 9:34 PM
  • I don't think WA_TranslucentBackground is necessary
  • Change newline handling
abalaji marked an inline comment as done.Nov 29 2018, 9:35 PM
ngraham added inline comments.Nov 30 2018, 3:20 AM
src/QuickEditor/QuickEditor.cpp
39

Hmm, we still can't find a better way to handle this? I'm not sure if this text is really translatable in its current state.

abalaji added inline comments.Nov 30 2018, 9:20 PM
src/QuickEditor/QuickEditor.cpp
39

@ngraham I'm working on a better way to do this

abalaji marked 2 inline comments as done.Nov 30 2018, 9:21 PM
abalaji updated this revision to Diff 47555.Dec 14 2018, 10:41 AM
  • Fix help text
abalaji marked 2 inline comments as done.Dec 14 2018, 10:43 AM

@ngraham sorry the past two weeks were quite busy for me, I've been travelling. I finally got some time to fix the help text. No more hacks like &nbsp

abalaji updated this revision to Diff 47556.Dec 14 2018, 11:06 AM
  • Add my name to copyright
abalaji updated this revision to Diff 47561.Dec 14 2018, 1:28 PM
  • Cleanup
ngraham accepted this revision.Dec 14 2018, 2:17 PM

I'm satisfied now. @broulik?

This revision is now accepted and ready to land.Dec 14 2018, 2:17 PM

I tested on current master it and it compiles and works; in several capture modes, but only on a single monitor (3840x2160). I can test later (on monday) with dual monitor setup.

src/QuickEditor/QuickEditor.h
89–96

Some constants are initialized in .cpp-file, some here in header, why so inconsistent?
And this line is also using constexpr, while others don't

abalaji added inline comments.Dec 16 2018, 2:10 PM
src/QuickEditor/QuickEditor.h
89–96

So, I'm trying to keep all constants in the .cpp file since they are implementation details. But, since bottomHelpText is an array, and the length of an array is necessary (variable-length arrays not allowed in C++), I'm forced to leave this constant in the header file. constexpr is not really needed here tho, so I can make it just const.

abalaji updated this revision to Diff 47707.Dec 17 2018, 2:12 AM
abalaji marked 2 inline comments as done.
  • Change constexpr to const
alexeymin accepted this revision.Dec 17 2018, 10:40 AM

Works here with 2 monitors like this, tested all modes, seems OK.

broulik accepted this revision.Dec 17 2018, 3:03 PM
ngraham accepted this revision.Dec 17 2018, 3:06 PM
This revision was automatically updated to reflect the committed changes.

Now the translation for these lines does not work.

{QStaticText(i18n("Enter, double-click:")), {QStaticText(i18n("Take screenshot"))}},
{QStaticText(i18n("Shift:")), {
    QStaticText(i18n("Hold to toggle magnifier")),
    QStaticText(i18n("while dragging selection handles"))
}},
{QStaticText(i18n("Arrow keys:")), {
    QStaticText(i18n("Move seletion rectangle")),
    QStaticText(i18n("Hold Alt to resize, Shift to fine‑tune"))
}},
{QStaticText(i18n("Right-click:")), {QStaticText(i18n("Reset selection"))}},
{QStaticText(i18n("Esc:")), {QStaticText(i18n("Cancel"))}},

};

Darn. Is it because they're inside QStaticText()? @abalaji, could you take a look?

Probably because it is constructed globally before QCoreApplication / before translations were set up, or something like that

Yeah, you cannot have i18n in a global static

Oh, I see, I'll just move them into the constructor