Add new button "Record screen" that allows to select a Screen Recording tool
ClosedPublic

Authored by gregormi on Feb 4 2018, 1:56 PM.

Details

Summary

Adds a new button "Record screen" that allows to select a screen recording tool, currently Peek and SimpleScreenRecorder (and soon vokoscreen: https://phabricator.kde.org/D10390)

Moved from Reviewboard: https://git.reviewboard.kde.org/r/130215/

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.
gregormi requested review of this revision.Feb 4 2018, 1:56 PM
gregormi created this revision.
gregormi edited the summary of this revision. (Show Details)Feb 4 2018, 1:58 PM

Does Spectacle belong to a certain review group?

ngraham edited reviewers, added: Spectacle; removed: bgupta.Feb 4 2018, 3:04 PM

Yup!

This looks great! We're currently in the process of re-doing the buttons on the bottom (T7841), so let's get that done fist and then we can investigate this.

rkflx added a subscriber: rkflx.Feb 7 2018, 8:49 PM

This looks great! We're currently in the process of re-doing the buttons on the bottom (T7841), so let's get that done fist and then we can investigate this.

The important bits are now done in D10371 (either wait until it landed, or "Depend on" it).

Do KMoreTools allow to add it like this?:

  • Print
  • ---------
  • Record screen with Peek
  • Record screen with SimpleScreenRecorder
  • Configure Tools...

We've got our tools menu now but it's looking a little bit sad with only a Print item. This would be a great time to rebase on master and put the screen recording items in there!

Also, I know some volks like Vokoscreen (ah-ha-ha); is there any way we can support that too, or is that up to KNewStuff or the Vokoscreen developer?

Do KMoreTools allow to add it like this?:

  • Print
  • ---------
  • Record screen with Peek
  • Record screen with SimpleScreenRecorder
  • Configure Tools...

I think it is possible with the current KMoreTools API but it the code not be nice. Better would be extend the the API for this use case.

One thing should be considered with the proposed menu layout: if the user chooses "Configure Tools..." this would only configure the "Record screen" tools and not also the Print tool. If one wanted to let the user configure the whole menu then everything should be moved to KMoreTools.

This alternative would work out of the box but comes with one more nesting in the menu:

  • Print
  • Record screen video
    • Peek
    • SimpleScreenRecorder
    • Vokoscreen
    • Configure Tools...

Also, I know some volks like Vokoscreen (ah-ha-ha); is there any way we can support that too, or is that up to KNewStuff or the Vokoscreen developer?

The "screenrecorder" grouping with Peek and SimpleScreenrecorder was added to KMoreTools with this patch (among other things): https://phabricator.kde.org/D7130. Vokoscreen can be added the same way. So it is up to KNewStuff/KMoreTools.

gregormi edited the summary of this revision. (Show Details)Feb 8 2018, 4:03 PM
gregormi updated this revision to Diff 26771.Feb 8 2018, 4:07 PM
  • Move "Record Screen Video" to Tools menu (see screenshot)

Instead of "Record Screen Video", how about "Make Screen Recording" or just "Record screen"?

Instead of "Record Screen Video", how about "Make Screen Recording" or just "Record screen"?

I thought that just "Record screen" could be confused with to take (or record) a still image of the screen. But I am no native speaker. So, you decide. :-)

"Record" in English always has a time component when talking about some form of media. You would record a video, but you wouldn't record an image.

FYI You can update the screenshot in the summary section if you'd like.

gregormi edited the summary of this revision. (Show Details)Feb 8 2018, 4:29 PM
rkflx added a comment.Feb 8 2018, 4:29 PM

Here's how it looks for me:

I'd really love to omit one level of extra nesting.

Do KMoreTools allow to add it like this?:

  • Print
  • ---------
  • Record screen with Peek
  • Record screen with SimpleScreenRecorder
  • Configure Tools...

I think it is possible with the current KMoreTools API but it the code not be nice. Better would be extend the the API for this use case.

Dependency Freeze for KDE Applications 18.04 is on March 15, meaning you'd have to get that done for KDE Frameworks 5.44 on March 3. Do you think you could do that? If not, I guess the current solution would be fine. What would "not nice code" entail?

if the user chooses "Configure Tools..." this would only configure the "Record screen" tools and not also the Print tool.

I know, that's why I proposed to have the horizontal separator. Of course then there should be no separator between the screen recording tools and the configure button ;)

Do users actually have both tools installed? If not, it's showing "More" anyway instead of "Configure".

Also: "Configure" clashes with the regular Configure button. Could you rename it to something like Configure external tools?

Instead of "Record Screen Video", how about "Make Screen Recording" or just "Record screen"?

Yeah, that's what I stumbled upon too. Wikipedia has it as "Screencasting", but "Record screen" is probably fine.

gregormi updated this revision to Diff 26773.Feb 8 2018, 4:30 PM
  • Menu title is now "Record Screen" again.
rkflx added a comment.Feb 8 2018, 4:50 PM

nesting

You could also think about QMenu::addSection.

Here's how it looks for me:

I'd really love to omit one level of extra nesting.

Yes, I know KMoreTools comes with too much nesting in menus :-(. Especially when there are few items it not really looks good.

I think it is possible with the current KMoreTools API but it the code not be nice. Better would be extend the the API for this use case.

Dependency Freeze for KDE Applications 18.04 is on March 15, meaning you'd have to get that done for KDE Frameworks 5.44 on March 3. Do you think you could do that? If not, I guess the current solution would be fine. What would "not nice code" entail?

Meeting the deadline: no. I would have to familiarize again with the current API and come up with a good ideas.

"not nice code": I designed KMoreTools to be very flexible but sadly this comes with a complicated API. Up to now, this flexibility was not used and I think it would be better to deprecate some of the API and add better ones. E.g. currently, the relevant line in Spectacle is:

this->mScreenrecorderToolsMenuFactory->fillMenuFromGroupingNames(mScreenRecorderToolsMenu, { QStringLiteral("screenrecorder") });

This could be replaced with something like this:

this->mScreenrecorderToolsMenuFactory->fillMenuWithPrefixedItemsFromGroupingNames(mScreenRecorderToolsMenu, i18n("Record screen with "), { QStringLiteral("screenrecorder") });

if the user chooses "Configure Tools..." this would only configure the "Record screen" tools and not also the Print tool.

I know, that's why I proposed to have the horizontal separator. Of course then there should be no separator between the screen recording tools and the configure button ;)

Sure :)

Do users actually have both tools installed? If not, it's showing "More" anyway instead of "Configure".

Yes, the idea was that "Configure" should be as unobstrusive as possible. I even thought about hiding it completely and only show it when the user presses a key like "Shift". Some discussions are needed to make this better :)

Also: "Configure" clashes with the regular Configure button. Could you rename it to something like Configure external tools?

From KMoreTools' view: the question here would be if the name "Configure" should be configurable or if it should be globally renamed.

Instead of "Record Screen Video", how about "Make Screen Recording" or just "Record screen"?

Yeah, that's what I stumbled upon too. Wikipedia has it as "Screencasting", but "Record screen" is probably fine.

This can be changed anytime.

gregormi updated this revision to Diff 26777.Feb 8 2018, 5:06 PM
  • Use AddSection and rework Tools menu to remove nesting

I underestimated the current API :). Now it looks not really perfect but much better:

gregormi edited the summary of this revision. (Show Details)Feb 8 2018, 5:09 PM
rkflx added a comment.Feb 8 2018, 5:39 PM

Cool! I'm still not 100% convinced what variant to ship for 18.04, though.

@ngraham How do you like it now?

With no tools available at all it looks a bit weird, but I guess that's also something that should be improved in KMoreTools eventually (along with prefixing, separate calls for app entries vs. "management" entries, more flexible naming, less nesting, integration with Discover etc.).

From KMoreTools' view: the question here would be if the name "Configure" should be configurable or if it should be globally renamed.

Idea: Edit Entries – This should reduce the chances of clashing greatly. Apart from that, this depends on where it is used currently and where you want to use it in the future (I cannot answer that ;).

The centered "Record Screen" title in the menu is somehow bothering me. Doesn't feel quite right. Might be nicer if it were left-aligned, had a colon on the end of it, and it was grayed out. That way it would look more like a header introducing a section and less like a weird menu item.

rkflx added a comment.Feb 8 2018, 5:52 PM

The centered "Record Screen" title in the menu is somehow bothering me. Doesn't feel quite right. Might be nicer if it were left-aligned, had a colon on the end of it, and it was grayed out. That way it would look more like a header introducing a section and less like a weird menu item.

I fear that's how Breeze works…

Let's step back a bit: What's the common case? None of the apps is installed, I guess (correct me if I'm wrong). Then it looks bad, so I'd say for now we use the nested menu, and improve things in KMoreTools before making Spectacle better.

Thoughts?

The centered "Record Screen" title in the menu is somehow bothering me. Doesn't feel quite right. Might be nicer if it were left-aligned, had a colon on the end of it, and it was grayed out. That way it would look more like a header introducing a section and less like a weird menu item.

I fear that's how Breeze works…

OK, you're right. I notice the same pattern used in Kate. Let;s not tinker with it; consistency is more important.

Let's step back a bit: What's the common case? None of the apps is installed, I guess (correct me if I'm wrong).

Yes, I agree that we need to optimize for this case. Does KNS open Discover with the app in question if you opt to install it?

The centered "Record Screen" title in the menu is somehow bothering me. Doesn't feel quite right. Might be nicer if it were left-aligned, had a colon on the end of it, and it was grayed out. That way it would look more like a header introducing a section and less like a weird menu item.

Me too.

I fear that's how Breeze works…

I also thought that.

Let's step back a bit: What's the common case? None of the apps is installed, I guess (correct me if I'm wrong). Then it looks bad, so I'd say for now we use the nested menu, and improve things in KMoreTools before making Spectacle better.

No apps installed:

Thoughts?

rkflx added a comment.Feb 8 2018, 5:59 PM

Yes, I agree that we need to optimize for this case. Does KNS open Discover with the app in question if you opt to install it?

For me it will open the browser, see D10295#203095. Do you see any appstream URL in D10390?

The centered "Record Screen" title in the menu is somehow bothering me. Doesn't feel quite right. Might be nicer if it were left-aligned, had a colon on the end of it, and it was grayed out. That way it would look more like a header introducing a section and less like a weird menu item.

Me too.

I fear that's how Breeze works…

I also thought that.

Let's step back a bit: What's the common case? None of the apps is installed, I guess (correct me if I'm wrong). Then it looks bad, so I'd say for now we use the nested menu, and improve things in KMoreTools before making Spectacle better.

No apps installed:

Thoughts?

The nesting looks fine to me. There are two visual papercuts I'd like to see addressed, and then I'd +1 that for the "no apps installed" case:

  • Double lines above "More"
  • The word "More" is not very descriptive. For now, since the only thing it lets you do is download screen recording tools, how about changing that to "Record screen", which will give a preview of the available functionality?

Yes, I agree that we need to optimize for this case. Does KNS open Discover with the app in question if you opt to install it?

For me it will open the browser, see D10295#203095. Do you see any appstream URL in D10390?

That's pretty useless then; Linux users don't install apps from random websites! We need to fix this in KNewStuff!

Let's step back a bit: What's the common case? None of the apps is installed, I guess (correct me if I'm wrong).

Yes, I agree that we need to optimize for this case. Does KNS open Discover with the app in question if you opt to install it?

I also see this as a problem. However, I am not sure if I currently have the time to work on KMoreTools to help to make it better. But this would mean we cannot get the Record Screen feature shipped which I find not really satisfactory.

Yes, I agree that we need to optimize for this case. Does KNS open Discover with the app in question if you opt to install it?

For me it will open the browser, see D10295#203095. Do you see any appstream URL in D10390?

That's pretty useless then; Linux users don't install apps from random websites! We need to fix this in KNewStuff!

Definitely. Discover integration would be the killer feature.

Yes, I agree that we need to optimize for this case. Does KNS open Discover with the app in question if you opt to install it?

For me it will open the browser, see D10295#203095. Do you see any appstream URL in D10390?

There is none (appstream URL), because at that time I had no idea of how to do it in a good way.

That's pretty useless then; Linux users don't install apps from random websites! We need to fix this in KNewStuff!

Definitely. Discover integration would be the killer feature.

Maybe, the website should still be present if there is no appstream info yet.

rkflx added a comment.EditedFeb 8 2018, 6:10 PM

@ngraham It's either addSection with the bugs, or nesting. All other changes (e.g. to get it to look like my original proposal in D10295#202617) are not possible inside Spectacle safe for ugly hacks.

My vote for 18.04 is for nesting.

I won't argue, nesting is fine.

So back to nesting?

I'm not a huge fan, but that's the more standard way of doing it (See Dolphin's Free Space widget in the bottom right corner), so if we improve KNS to have a better UI, spectacle should be able to just inherit that.

rkflx added a comment.EditedFeb 8 2018, 6:16 PM

So back to nesting?

I guess so. At least we tried, and identified what needs to be improved.

@gregormi It's fine if you are short on time to work on this, but it would be a shame if the ideas got lost in a random Phabricator comment. Could you open tasks on some Phab workboard to at least track this?

So back to nesting?

I guess so. At least we tried, and identified what needs to be improved.

@gregormi It's fine if you are short on time to work on this, but it would be a shame if the ideas got lost in a random Phabricator comment. Could you open tasks on some Phab workboard to at least track this?

I did not work with Phabricator workboards before. Is there a specific one where I could add these ideas?

rkflx added a comment.Feb 8 2018, 6:36 PM

I did not work with Phabricator workboards before. Is there a specific one where I could add these ideas?

Sorry, I should've done the work of looking for a board. As KNewStuff has its own project on Phab, just use the one over there: https://phabricator.kde.org/project/view/127/.

Thanks ;)

rkflx added a comment.Feb 8 2018, 7:45 PM

(Review of Diff 26773.)

src/Gui/KSMainWindow.cpp
156

You could use the three-argument connect by removing the last mScreenRecorderToolsMenu.

158

Is this-> needed?

rkflx added a comment.Feb 8 2018, 8:46 PM

Forgot something: It would be great if you could add a short mention of your new feature to doc/index.docbook (grep for <guibutton>Tools</guibutton>).

gregormi updated this revision to Diff 26942.Feb 11 2018, 5:22 PM
gregormi marked 2 inline comments as done.
  • Add documentation in index.docbook and minor improvements in the same section
  • fix code according to suggestions
  • Revert "Use AddSection and rework Tools menu to remove nesting": now the menu is nested again
ngraham accepted this revision.Feb 11 2018, 8:01 PM
This revision is now accepted and ready to land.Feb 11 2018, 8:01 PM
rkflx accepted this revision.Feb 11 2018, 8:01 PM

Thanks again. Small docbook nitpicks, then it can land ;)

doc/index.docbook
198

Starting those text parts with a lowercase letter each looks a bit odd in KHelpCenter. Better start with an uppercase letter.

(Losing "This button" everywhere is fine, I think.)

210–218

Technical issue: The indentation is a bit off, because sadly this file uses tabs instead of spaces. Please adapt it (I stumbled over this in my patch too…).

rkflx added a comment.Feb 11 2018, 9:37 PM

@gregormi D10301 just landed, you might want to watch out for docbook merge conflicts.

@gregormi Just so you know, this is the last patch that needs to merge before I can publish a Usability & Productivity blog post about all the awesome Spectacle changes recently!

@rkflx: May I ask what is your toolchain when dealing with docbook? I found out this:

cd kde/src/kde/kdegraphics/spectacle/doc/ ; dblatex index.docbook ; xdg-open index.docbook.pdf

which gives a rough but usable preview.

When I click the Help button from the dev version of Spectacle, then KHelpCenter opens but it shows the German version of the Spectacle Handbook. But the application language of KHelpCenter is English.

gregormi updated this revision to Diff 27213.Feb 15 2018, 9:04 AM
gregormi marked 2 inline comments as done.
  • docbook: make first letters upper case; fix indentation
doc/index.docbook
210–218

This was caused because Kate used 8 spaces for tabs. I wonder why. I switched it to 4 spaces.

Why not convert the file use spaces instead of tabs?

This revision was automatically updated to reflect the committed changes.

[... ] As KNewStuff has its own project on Phab, just use the one over there: https://phabricator.kde.org/project/view/127/.

Thanks ;)

I created this task: https://phabricator.kde.org/T7971

@gregormi D10301 just landed, you might want to watch out for docbook merge conflicts.

Mhm, seems like we lost a bunch of changes. Not sure what's gone wrong, either bad editing of merge conflict or rebase on stale master instead of fresh origin/master?

Anyway, I'll open a Diff to fix this tonight. Then we can also figure out the best order of the tools menu's items, and move everything to // the tools menu.


@rkflx: May I ask what is your toolchain when dealing with docbook? I found out this:

cd kde/src/kde/kdegraphics/spectacle/doc/ ; dblatex index.docbook ; xdg-open index.docbook.pdf

which gives a rough but usable preview.

Well, I make install (all or just from doc) to a custom prefix with -DCMAKE_INSTALL_PREFIX=~/path, and then either press F1 in Spectacle or issue khelpcenter help:/spectacle/additional-functionality.html#id1580 (found via Copy Link Address). You need to set some paths for this to work (quite complicated, I know).

The PDF workflow is not bad, but unfortunately it misses some markup (e.g. keyboard shortcuts, menu items).

Related (directly dumped from my notes, because I stumbled upon this topic just like you a couple of weeks ago):

When I click the Help button from the dev version of Spectacle, then KHelpCenter opens but it shows the German version of the Spectacle Handbook. But the application language of KHelpCenter is English.

I just set my Plasma session to non-English and installed spectacle-lang to get translations. For make uninstall, I get the translated version, for make install I get the updated English version. AFAIK KHelpCenter relies on the help:/ kioslave, which looks in the standard paths like it should. Either move the relevant files there (/usr/share/... or ~/.local/...), or add your custom install prefix to the standard paths like in the link I mentioned.

Any volunteers to update some wiki pages? T7116#128201 suggests it's still a bit hard to find, or rather we need a developer-oriented cookbook for quick verification of docbook changes (as well as a better version of the env paths link)… @ngraham?


Thanks ;)

(Note you could simply omit https://phabricator.kde.org/, and then Phabricator will do the link for you as well as add a "mention" to the object you linked to, which makes it easier to find related things…)

@gregormi D10301 just landed, you might want to watch out for docbook merge conflicts.

Mhm, seems like we lost a bunch of changes. Not sure what's gone wrong, either bad editing of merge conflict or rebase on stale master instead of fresh origin/master?

You mean commits you made are lost because I did an $ arc land? Or are we talking about changes within this Differential revision?

Anyway, I'll open a Diff to fix this tonight. Then we can also figure out the best order of the tools menu's items, and move everything to // the tools menu.

ok.

rkflx added inline comments.Feb 15 2018, 12:47 PM
doc/index.docbook
210

You mean commits you made are lost because I did an $ arc land? Or are we talking about changes within this Differential revision?

It's not that bad ;) Just the Open Screenshots Folder blurb is missing. When you originally changed the docbook, it was not there, but in the meantime it was added to master.

Not sure what went wrong, but if you can figure it out we might add more tips for arc land on the wiki.

I identified some of the lost changes in index.docbook and added a new patch: https://phabricator.kde.org/D10542