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

VLCLibrary: debugLogging status (closes #186) #30

Closed
wants to merge 3 commits into from

Conversation

mkchoi212
Copy link
Contributor

@mkchoi212 mkchoi212 commented Jul 20, 2018

Fixes the bugs

  • debugLoggingStatus can not be set
  • debugLoggingLevel can exceed the set range of 0-4

VKLog(@"Invalid debugLoggingLevel of %d provided", debugLoggingLevel);
VKLog(@"Please provide a valid debugLoggingLevel between 0 and 4");
VKLog(@"Defaulting debugLoggingLevel to 4 (just errors)");
_debugLoggingLevel = 4;
Copy link
Contributor Author

@mkchoi212 mkchoi212 Jul 20, 2018

Choose a reason for hiding this comment

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

Decided to set a default value for the logging level because I think an invalid debug logging level isn't worth causing a crash.

Copy link
Contributor

Choose a reason for hiding this comment

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

The scale is the other way around - 0 is just errors, whereas 4 is "everything VLC ever wanted to tell you about anything". So the default should be 0.
However, there is something wrong if there is a crash when the user provides 5. Are you sure that this is happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! Also, it doesn't crash. What I meant was that I initially thought about doing fatalError("Invalid debugging level x") but just decided to warn the user about it.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about just capping the value between 0 and 4 meaning <0 to 0 >4 to 4 and if it's outside those bounds log the message ?

@mkchoi212 mkchoi212 force-pushed the lib-test branch 2 times, most recently from 4e1ed03 to 3aace59 Compare July 23, 2018 07:31
if (debugLogging) {
libvlc_log_set(_instance, HandleMessage, (__bridge void *)(self));
} else {
libvlc_log_unset(_instance);
}
}

- (void)setDebugLoggingLevel:(int)debugLoggingLevel
{
if (debugLoggingLevel < 0 && debugLoggingLevel > 4) {
Copy link
Contributor Author

@mkchoi212 mkchoi212 Jul 23, 2018

Choose a reason for hiding this comment

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

@carolanitz something like this?? I’m mildly confused :D

@mkchoi212 mkchoi212 mentioned this pull request Jul 26, 2018
@mkchoi212 mkchoi212 force-pushed the lib-test branch 2 times, most recently from ba827f3 to 2a1db09 Compare July 26, 2018 00:44
Copy link
Contributor

@fkuehne fkuehne left a comment

Choose a reason for hiding this comment

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

One nit-pick, rest looks good now! :)

* Currently, the framework does not support multiple instances of VLCLibrary. Furthermore, you cannot destroy any
* instiantiation of VLCLibrary, as previously noted, this is done automatically by the dynamic link loader.
* Currently, the framework __does not__ support multiple instances of VLCLibrary.
* Furthermore, you __cannot__ destroy any instance of VLCLibrary; this is done automatically by the dynamic link loader.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this underling syntax is permitted in doxygen. Can you verify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I had no idea this was run through doxygen. And turns out, doxygen only supports __ syntax for markdown.

@mkchoi212
Copy link
Contributor Author

ping 🔔

@carolanitz
Copy link
Member

merged

@carolanitz carolanitz closed this Aug 3, 2018
@mkchoi212 mkchoi212 deleted the lib-test branch August 7, 2018 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants