Differential D24102

Add Pause/Resume action for pausing and resuming speech synthesis.
AbandonedPublic
Actions

Authored by whiting on Sep 19 2019, 10:56 PM.

Details

Reviewers
aacid
Commits
R223:48633374a94e: Add Pause/Resume action for pausing and resuming speech synthesis.
Summary

Only enable the action when there's text being spoken or when speech is paused.
Also added ability to select the QtSpeech plugin/engine to use so it doesn't randomly choose one for us.

Test Plan

Pause and resume are working here.

Diff Detail

Repository
R223 Okular
Branch
pauseresumespeech
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17116
Build 17134: arc lint + arc unit
whiting created this revision.Sep 19 2019, 10:56 PM
Restricted Application added a project: Okular. · View Herald TranscriptSep 19 2019, 10:56 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
whiting requested review of this revision.Sep 19 2019, 10:56 PM
whiting edited the test plan for this revision. (Show Details)Sep 19 2019, 10:57 PM
whiting added a reviewer: aacid.
sander added a subscriber: sander.Sep 20 2019, 6:51 AM
sander added inline comments.
ui/pageview.cpp
2606 ↗(On Diff #66979)

Please avoid this whitespace change.

whiting updated this revision to Diff 66553.Sep 20 2019, 5:08 PM

Removed whitespace changes.

whiting marked an inline comment as done.Sep 20 2019, 5:08 PM
whiting updated this revision to Diff 66795.Sep 25 2019, 1:25 AM
  • Add user okular user setting for which QtSpeech engine to use.
whiting edited the summary of this revision. (Show Details)Sep 25 2019, 1:28 AM
whiting edited the test plan for this revision. (Show Details)
aacid added a comment.Sep 25 2019, 8:13 PM

Sounds sensible, what about the TODO you have on the top?

conf/dlgaccessibility.cpp
38

const

39

use for ( : )

whiting updated this revision to Diff 66860.Sep 25 2019, 10:30 PM

Changed engines stringlist to const and use for instead of foreach.

Also removed context menu action for pause resume

  • Add user okular user setting for which QtSpeech engine to use.
whiting updated this revision to Diff 66865.Sep 26 2019, 5:48 AM
  • Only enable Pause or Resume when speaking or paused.
whiting updated this revision to Diff 66866.Sep 26 2019, 6:10 AM
  • When tts engine is changed via okular settings recreate tts object.
whiting added a comment.Sep 26 2019, 9:53 PM
In D24102#537832, @aacid wrote:

Sounds sensible, what about the TODO you have on the top?

Done. I think this is ready to use now. When users change the engine in the config dialog the tts object stops any speech and recreates the underlying QTetxToSpeech object with the chosen engine. Stop action is also enabled when paused or speaking, the same as the pause/resume action.

whiting updated this revision to Diff 66979.Sep 28 2019, 4:55 AM
  • Fix building okular without QTextToSpeech by adding ifdefs.
sander added a comment.Sep 30 2019, 3:17 PM

Sorry for being a pain, but your patch contains lots of whitespace changes again (in conf/dlgaccessibilitybase.ui). I agree that you probably do want them changed, but please do so in a separate patch.

Besides, can you please post your patch as a merge request on https://invent.kde.org/kde/okular/ ? That is not purely cosmetic -- invent.kde.org has a CI system.

Besides that: I like the patch!

conf/dlgaccessibility.cpp
43

This doesn't follow the coding style in the rest of the file: There should be a whitespace right after '(' and before ')'.

whiting edited the summary of this revision. (Show Details)Oct 3 2019, 3:22 PM
In D24102#539859, @sander wrote:

Sorry for being a pain, but your patch contains lots of whitespace changes again (in conf/dlgaccessibilitybase.ui). I agree that you probably do want them changed, but please do so in a separate patch.

Besides, can you please post your patch as a merge request on https://invent.kde.org/kde/okular/ ? That is not purely cosmetic -- invent.kde.org has a CI system.

Besides that: I like the patch!

Ok, I'll fix the whitespace and move it to the other review tool, no problem.

whiting added a comment.Oct 7 2019, 5:00 PM
In D24102#541529, @whiting wrote:
In D24102#539859, @sander wrote:

Sorry for being a pain, but your patch contains lots of whitespace changes again (in conf/dlgaccessibilitybase.ui). I agree that you probably do want them changed, but please do so in a separate patch.

Besides, can you please post your patch as a merge request on https://invent.kde.org/kde/okular/ ? That is not purely cosmetic -- invent.kde.org has a CI system.

Besides that: I like the patch!

Ok, I'll fix the whitespace and move it to the other review tool, no problem.

Ok, removed whitespace changes that designer did for me and pushed to my clone and created new merge request here: https://invent.kde.org/kde/okular/merge_requests/51

aacid added a comment.Oct 7 2019, 6:02 PM

Can you please close this then?

whiting abandoned this revision.Oct 7 2019, 6:09 PM

Yep, Abandoning.

Showing Only Differences

This revision modifies 7 more files that are hidden because they were not modified between selected diffs and they have no inline comments.

Revision Contents
Changeset List

PathPackages
Mconf/dlgaccessibility.cpp (17 lines)

Diff 66979

View Options

conf/dlgaccessibility.cpp

Loading...