Add "move to device" functionality to hamburger menu
ClosedPublic

Authored by Fuchs on Feb 27 2018, 7:31 PM.

Details

Summary

This patch adds the possibility to choose a device for a playback / recording stream in the hamburger menu.
As per the discussion with the developers, the drag & drop functionality stays untouched and is still available.

This is added to be a bit more consistent with the kcm and to make the functionality easier to discover.

The menu is only shown when there are more than one possibilities to choose from in order to not confuse users.

FEATURE: 384292

Test Plan

Apply patch.
Have multiple output and input devices.
Move streams between devices and see if it works.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Fuchs created this revision.Feb 27 2018, 7:31 PM
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 27 2018, 7:31 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Fuchs requested review of this revision.Feb 27 2018, 7:31 PM
drosca requested changes to this revision.Feb 27 2018, 7:48 PM

I'm not sure on the wording "Output on" "Record from", I would prefer simple "Move to" or "Move to device", but I am not native speaker so I leave others to decide.

Can you merge the code into one block? As it is now, the code is duplicated and the only difference is that first block uses sourceView and second sinkView, but only one of those two blocks can ever be executed (can't be both sink-input and source-output).

applet/contents/ui/ListItemBase.qml
338

sinkView.model.count > 1 -> sinkView.count

This revision now requires changes to proceed.Feb 27 2018, 7:48 PM

I'm not sure on the wording "Output on" "Record from", I would prefer simple "Move to" or "Move to device", but I am not native speaker so I leave others to decide.

[puts on native English speaker hat]

Here are my recommendations:

  • "Output on" -> "Play audio using:" (or maybe "Transfer audio to", if we can guarantee that something is always playing when the user will see this)
  • "Record on" -> "Record audio using:"
Fuchs added a comment.Feb 27 2018, 7:57 PM

Can you merge the code into one block?

Technically yes, but due to some edge cases it will make it hacky and difficult to read.

Example: you can have a second device connected which is only an output (bluetooth speakers), but no input. Therefore before adding the menu entries you have to check whether there are currently available output or input types, depending on the situation.
So there will be a lot of conditionals, not just for setting labels, but for actual logic.

I'll see which is the least bad attempt at that and upload a second diff, but I am not conviced it will be nicer.

Fuchs updated this revision to Diff 28214.EditedFeb 27 2018, 8:17 PM

Updated the text, tried to merge the code blocks. Due to most of it happening in a for loop depending on the type, this was not possible in a sane way without having a helper method created.

Personally I think legibility goes a tad bit down. If made easier to read, performance would go suffer (e.g. executing parts of it despite it not going to be shown anyway).
I also think the .count variant is a bit easier to read, but take whichever you prefer.

Text used without colon (:) in order to be consistent with "ports"

Fuchs updated this revision to Diff 28218.Feb 27 2018, 8:35 PM
Fuchs marked an inline comment as done.

According to discussion on IRC, sacrifice some more readability for brevity.

At this point, let me also mention that screenshots are not only much appreciated, but vastly increase the chance of your patch (and name!) being featured in the next week's installment of https://pointieststick.wordpress.com/category/usability-productivity/, especially for a much-requested feature like this one! :)

+1. No more code comments.

Just a note, we can't guarantee that audio is actually playing from that stream.

applet/contents/ui/ListItemBase.qml
338

Sorry about this comment, it's nonsense. It was fine in your fist diff.

It must check > 1 not just > 0.

Fuchs added a comment.EditedFeb 27 2018, 8:51 PM

At this point, let me also mention that screenshots are not only much appreciated, but vastly increase the chance of your patch (and name!) being featured in the next week's installment of https://pointieststick.wordpress.com/category/usability-productivity/, especially for a much-requested feature like this one! :)

I'm not sure whether it is worth a mention and I think I'm the only one requesting it so far (might be wrong), but if you want to add me: sure. Screenshot above (assuming this worked, I am new to phab), the name is Fuchs. Thanks. And sorry for the misunderstanding.

Fuchs updated this revision to Diff 28219.Feb 27 2018, 9:03 PM

Move text back to if/else for at least a bit of readability, reverted the check to go for > 1 since that is what we are interested in. Final revision, hopefully.

You're definitely not the only one who wants this. See:

In fact, because this implements that feature, would you mind adding "FEATURE: 384292" into the Summary section on its own line, below other text? That's a special keyword that will make that bug get closed once this lands. See also https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords

Fuchs edited the summary of this revision. (Show Details)Feb 27 2018, 9:05 PM
Fuchs marked an inline comment as done.Feb 27 2018, 9:08 PM

You're definitely not the only one who wants this. See:

Yeah, I read about these, I was simply too lazy to see if someone wanted that. Anyway, added in the summary plus commented on the bug report.

The screenshot now is lost somehwere in phab backlog as it is no longer regarding the latest revision, but I assume you can find that :)

drosca added inline comments.Feb 27 2018, 9:10 PM
applet/contents/ui/ListItemBase.qml
340

if (

343

} else { on the same line

350

extra newline

@ngraham This is not a new feature, only people didn't know about the drag&drop functionality, which is understandable, but nevertheless the functionality is there for a long time already (since around Plasma 5.7/5.8).

Fuchs updated this revision to Diff 28221.Feb 27 2018, 9:13 PM

Adapt code style.

With regards to wording: even if we can't guarantee that audio is playing / being recorded, the wording is still correct.
Since if there will be audio played / recorded, it will be on the device the user chooses. So it makes sense.

ngraham edited the test plan for this revision. (Show Details)Feb 27 2018, 9:56 PM
drosca accepted this revision.Feb 28 2018, 12:12 PM
This revision is now accepted and ready to land.Feb 28 2018, 12:12 PM
This revision was automatically updated to reflect the committed changes.