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

Add deflect::Observer which can be used to only receive events w/o the need to send images #175

Merged
merged 2 commits into from
Aug 2, 2017

Conversation

tribal-tec
Copy link
Contributor

No description provided.

@tribal-tec tribal-tec changed the title Add deflect::Observer which can be used only receive events w/o the need so send images Add deflect::Observer which can be used to only receive events w/o the need to send images Jul 28, 2017
class StreamPrivate;

/**
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty docstring

adjust documentation in this file a bit, explaining how to use an Observer and replacing "stream" with "observer" where applicable

{
}

FrameDispatcher* frameDispatcher;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo we be deleted

Stream::Future StreamSendWorker::enqueueObserverClose()
{
return _enqueueRequest(
{[this] { return _send(MESSAGE_TYPE_OBSERVER_QUIT, {}); }});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a separate MESSAGE_TYPE_OBSERVER_QUIT? Couldn't we just use the normal MESSAGE_TYPE_QUIT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't find a satisfying solution to make that work. Now it's clearer having two messages indicating the the source is a observer.

emit removeStreamSource(_streamId, _sourceId);
emit removeObserver(_streamId, _sourceId);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look like a bit brute force, maybe remember the stream type and emit just the correct signal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, if the type is remembered, the upper comment could also be resolved. I'll have a look

CMakeLists.txt Outdated
@@ -4,7 +4,7 @@
# Daniel Nachbaur <daniel.nachbaur@epfl.ch>

cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
project(Deflect VERSION 0.13.1)
project(Deflect VERSION 0.14.0)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the split of Stream in Observer + Stream to break the ABI as well, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably yes. Will bump for safety.

Copy link

@rdumusc rdumusc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last thing I don't know is if we should update the @Version tags in Observer.h, now that they are in a new class. Both updating and keeping them as is seems a bit wrong.

@tribal-tec
Copy link
Contributor Author

I'm actually wondering what the version tag corresponds to the CMakeLists.txt version. 0.13 == 1.3? But there's a 1.5 version tag in Observer.h as well...
Maybe mention that this class is new since version 1.5, but for the methods we keep the version we had as it didn't break looking from the Stream's point-of-view.

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