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

Log #61

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from
Open

Log #61

wants to merge 26 commits into from

Conversation

YruamaLairba
Copy link
Contributor

I started to work on the log spec.

@Philipp91
Copy link
Contributor

Ah this looks like the solution to #91. Could you add a usage example (perhaps in the dummy unit test)?

@YruamaLairba
Copy link
Contributor Author

Ah this looks like the solution to #91. Could you add a usage example (perhaps in the dummy unit test)?

sorry, this branch is far behind the rest of the main branch. I don't remember the details, but i stopped this branch because at the moment, there is no way to implement fully an properly in pure rust. If you need it to display some debug information during plugin development, you can use "println!" instead and run the host from a console.

@Philipp91
Copy link
Contributor

Thanks!

@prokopyl
Copy link
Member

prokopyl commented Aug 9, 2021

Hi @YruamaLairba , do you still want to work on this PR?

While I agree full realtime formatting support is not easy (and, in fact, may very well be the job of a whole separate crate), I believe a simple function that just sends a nul-terminated string as a log to the host would be quite useful already. We can add the formatting part later. 🙂

@prokopyl prokopyl added the 🎁 New LV2 Spec Implements a new LV2 Specification label Aug 9, 2021
@YruamaLairba
Copy link
Contributor Author

Lol, i read the Log spec API again, and in general, this should not be used in real-time context (does the doc have been updated ?). The only exception is for debugging purpose (trace) where messing real-time is not a concern.
So formatting from plugin side instead of host side doesn't seems to be a real issue. I also think it's acceptable to support trace in real-time context through an optional feature.
i think i will look at it soon since only few change are required

@prokopyl
Copy link
Member

prokopyl commented Aug 9, 2021

Yes, I agree, realtime formatting doesn't seem like a very big concern for now. ^^
I think having a simple API like the one you have implemented already that just logs a C string, and letting users in charge of formatting/acllocations should be enough, as I believe most errors would happen during instantiation or other non-realtime contexts.

Copy link
Member

@prokopyl prokopyl left a comment

Choose a reason for hiding this comment

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

I'll also do a quick review here, since you plan on continuing the work you may find this helpful 🙂

log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated
use std::os::raw::*; //get all common c_type
use urid::*;

pub struct EntryClass;
Copy link
Member

Choose a reason for hiding this comment

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

I think having the entry types in a submodule (entries for instance) would be a cleaner, although it's not a lot of code, it'll show up as quite a few types in the rustdoc and might be a bit overwhelming for new users that might want to focus elsewhere (mainly the Log feature, or the entry type trait).

log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
@YruamaLairba
Copy link
Contributor Author

@prokopyl wait a minute, i just looked in again and it appear i have some unpushed work in my local dir. I will do a push force with my work on the top the develop branch for a cleaner git history.

I also reminded a big interrogation i had about threading class of this feature. This feature is allowed in any non-realtime context, i think this implies the "worker" thread, but i really don't see how to implement that without messing the principle of separation of LV2 extension.

@YruamaLairba
Copy link
Contributor Author

Sorry, rebasing was a very bad idea, i didn't see the code wasn't compiling at all after therebase

@YruamaLairba
Copy link
Contributor Author

except documentation, i don't see what more i can do...

@YruamaLairba
Copy link
Contributor Author

@prokopyl have you an opinion on code i did?

log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
log/src/lib.rs Show resolved Hide resolved
log/src/lib.rs Outdated Show resolved Hide resolved
@prokopyl
Copy link
Member

@prokopyl have you an opinion on code i did?

@YruamaLairba Yes! Sorry for the wait, I just did another review. 🙂

Also, don't bother with making extensive documentation for now, I'll merge this into a log branch on this repo so that it can be written later!

@YruamaLairba
Copy link
Contributor Author

I think it's almost, it's only need documentation.

I wouldn't do submodule, there is too few type in my opinion.

@YruamaLairba YruamaLairba marked this pull request as ready for review August 21, 2021 16:49
@YruamaLairba
Copy link
Contributor Author

I completed and reworked the documentation.

I also removed the unsafety of the Entry trait. This is because the Log spec allow to define and use additional log level (i missed this earlier). I think this imply host should well behave when attempting to log with an unrecognized urid. I also add the URIbound as trait bound for Entry.

@YruamaLairba
Copy link
Contributor Author

@prokopyl you may didn't see, i'm ready for a review since a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎁 New LV2 Spec Implements a new LV2 Specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants