Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added DisplayText DBUS support #595

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Mello-Yello
Copy link

I added the DisplayText to the Root Interface, because I felt that the Player Interface had more to do about controlling the playback.

To avoid passing the m_osd variable directly to OMXControl, which felt cumbersome, I decided to pass it to OMXPlayerSubtitles. This way OMXPlayerSubtitles.DisplayText() checks whether it's true, and prints the text if that's the case.

I also updated the README and dbuscontrol.sh accordingly.

#587

@torarin
Copy link

torarin commented Dec 23, 2017

m_osd has to be initialized to false in the OMXPlayerSubtitles for this to work. However, this is not quite in line with how OMXPlayerSubtitles otherwise works. Most of the functions check m_open to see if Open has been called. Now in DisplayText this is bypassed by the m_osd test. Perhaps you could just make a lambda to send to OMXControl that checks m_osd?

@torarin
Copy link

torarin commented Dec 23, 2017

I see now that this a general problem in OMXControl. Calling SetVisible for instance with no subtitles and m_osd = false will trigger the assertion .

@Mello-Yello
Copy link
Author

If that's the case would it be better to pass m_osd to OMXControl, so we can also add a check for SetVisible and undo the changes to OMXPlayerSubtitles?

@torarin
Copy link

torarin commented Dec 23, 2017

That would work. The most practical thing would probably be to change the asserts in the functions relevant to OMXControl to if (!m_open) return, and remove the NDEBUG guards around m_open.

@Mello-Yello
Copy link
Author

Ok, I'll take a look at those assertions later

@torarin
Copy link

torarin commented Dec 24, 2017

Looks good. There's also GetActiveStream and SetActiveStream.

@Mello-Yello
Copy link
Author

Mello-Yello commented Dec 24, 2017

Just one thing: GetActiveStream must return a size_t value.
Would a -1 do it?

Also (I'm new to Git and Github), all this commits will count as one when merged? Otherwise, would it be better If I made another fork so I can make a pull request with just one commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants