Use logging crate #135

LiHRaM · 2019-08-31T09:18:01Z

fixes #99

Looking to start contributing again, thought this might be a nice place to start.

I've added logging using the log crate.

Todo:

Replace calls to println and eprintln with appropriate logging macros
Add example with logging impl

cmyr

Awesome, thanks for this; definitely something I've wanted and have been avoiding getting around to, so very much appreciated!

have some little notes on logging levels and a thought on your example, but overall very happy for this work!

cmyr · 2019-09-03T01:13:48Z

druid-shell/src/mac/mod.rs

@@ -204,7 +206,7 @@ lazy_static! {
        }
        decl.add_method(sel!(dealloc), dealloc as extern "C" fn(&Object, Sel));
        extern "C" fn dealloc(this: &Object, _sel: Sel) {
-            eprintln!("view is dealloc'ed");
+            error!("view is dealloc'ed");


I think this is just info, deallocation is part of normal program execution.

cmyr · 2019-09-03T01:16:22Z

src/localization.rs

@@ -149,7 +151,7 @@ impl ResourceManager {
    /// Return the best localization bundle for the provided `LanguageIdentifier`.
    fn get_bundle(&mut self, locale: &LanguageIdentifier, resource_ids: &[String]) -> BundleStack {
        let resolved_locales = self.resolve_locales(locale.clone());
-        eprintln!("resolved: {}", PrintLocales(resolved_locales.as_slice()));
+        info!("resolved: {}", PrintLocales(resolved_locales.as_slice()));


I vote debug!.

cmyr · 2019-09-03T01:16:30Z

src/localization.rs

@@ -213,7 +215,7 @@ impl L10nManager {
            .parse()
            .unwrap_or_else(|_| default_locale.clone());
        let locales = get_available_locales(base_dir).unwrap_or_default();
-        eprintln!(
+        info!(


cmyr · 2019-09-03T01:19:02Z

src/localization.rs

@@ -265,7 +267,7 @@ impl L10nManager {
            .current_bundle
            .format_pattern(key, value, args, &mut errs);
        for err in errs {
-            eprintln!("localization error {:?}", err);
+            error!("localization error {:?}", err);


I vote warn!. This is tricky; but basically error! is the highest log level we have, and should probably be reserved for things that are fatal or otherwise compromise the execution of the application. This is maybe an error though, it's hard to say. I think that this summary is a reasonable overview of how to think about the different log levels, this is definitely a 'feel' thing, though!

Yeah. I just ventured a guess with most of them, but didn't feel familiar enough with the internals of the library in a lot of cases. Thanks for taking the time. 😄

cmyr · 2019-09-03T01:20:02Z

src/widget/textbox.rs

@@ -193,11 +195,11 @@ impl TextBox {

    // TODO: waiting on druid clipboard support for copy / paste.
    fn copy_text(&self, input: String) {
-        eprintln!("COPY: {}", input);
+        error!("COPY: {}", input);


I'd go with warn!, and change the message to indicate this is not implemented. 👍

cmyr · 2019-09-03T01:20:34Z

src/lib.rs

@@ -764,7 +766,7 @@ impl<T: Data + 'static> WinHandler for UiMain<T> {
    }

    fn command(&mut self, id: u32, ctx: &mut dyn WinCtx) {
-        eprintln!("got command {}", id);
+        error!("got command {}", id);


maybe debug!?

cmyr · 2019-09-03T01:28:52Z

examples/logging.rs

+// Typically the user would use a crate for logging
+// Here we implement the Log trait for a struct and print everything to STDOUT
+// See https://docs.rs/log/ for more info
+use log::{Level, Metadata, Record};


I appreciate the thoroughness, but I wonder if this isn't out of scope for this PR?

What I would personally be inclined to do is to find the simplest, most minimal logging crate ([simple_logger]](https://github.com/borntyping/rust-simple_logger) was the best i could find in a quick search) and just use that? It's closer to what the user will ever actually do.

Something we might consider for follow-up work is to add something like this to druid itself, and then we could let the user ask for stderr logging when they start the runloop.

I wrote it like that to avoid any extra dependencies. Little did I know about this...

So I added it as a dev-dependency.
Anyway, sounds good. I'll leave more concrete logging implications on the druid library up to another PR.

yea, dev dependency is perfect. 👍

cmyr · 2019-09-03T01:29:21Z

druid-shell/src/windows/mod.rs

@@ -513,7 +515,7 @@ impl WndProc for MyWndProc {
                    let text = match s.stashed_char {
                        Some(c) => c,
                        None => {
-                            eprintln!("failed to convert WM_CHAR to char: {:#X}", wparam);
+                            error!("failed to convert WM_CHAR to char: {:#X}", wparam);


warn! I think.

cmyr · 2019-09-03T01:29:33Z

druid-shell/src/windows/mod.rs

@@ -865,7 +867,7 @@ impl WindowBuilder {
            }

            let dcomp_state = create_dcomp_state(self.present_strategy, hwnd).unwrap_or_else(|e| {
-                println!("Error creating swapchain, falling back to hwnd: {:?}", e);
+                error!("Error creating swapchain, falling back to hwnd: {:?}", e);


warn! I think?

cmyr · 2019-09-03T01:30:01Z

druid-shell/src/windows/mod.rs

@@ -930,9 +932,9 @@ unsafe fn create_dcomp_state(
            &IID_IDXGIFactory2,
            &mut factory as *mut *mut IDXGIFactory2 as *mut *mut c_void,
        ))?;
-        println!("dxgi factory pointer = {:?}", factory);
+        info!("dxgi factory pointer = {:?}", factory);


these can all be debug!.

cmyr

Awesome, thanks. A few more little things but this looks good.

cmyr · 2019-09-03T08:54:33Z

Cargo.toml

@@ -1,27 +1,31 @@
-[package]


do you have any idea why this is showing all these changes? Did the encoding change or something??

edit: looked into this, and it's a different line ending?

Weird. My git settings should be enforcing LF. Sorry about that

cmyr · 2019-09-03T08:55:38Z

src/widget/null.rs

-        true
-    }
-}
+// Copyright 2018 The xi-editor Authors.


ditto here, something confusing is going on. I wonder if it's just the diff is screwing up..

cmyr · 2019-09-03T08:58:59Z

examples/logging.rs

+use simple_logger;
+
+fn main() {
+    simple_logger::init().unwrap();


I actually think we can get rid of this file, but I would like to add logging to a few examples; in particular hello, calc, and custom_widget. (These are the examples I mostly use when experimenting & debugging)

Fair enough. ;) I've added logging to those examples.

Added logging to three other examples instead

cmyr

Awesome, thanks for this @LiHRaM!

LiHRaM force-pushed the logging branch from f10fb79 to 3fa787e Compare August 31, 2019 09:20

Add logging via log crate

50a7c88

LiHRaM force-pushed the logging branch from 6dc1265 to 50a7c88 Compare August 31, 2019 11:40

cmyr reviewed Sep 3, 2019

View reviewed changes

LiHRaM added 2 commits September 3, 2019 09:29

Adjust log levels, add simple_logger as dev-dep

c3e4c66

adjust logging levels

60f0587

cmyr reviewed Sep 3, 2019

View reviewed changes

CRLF -> LF, remove logging.rs

1feb624

Added logging to three other examples instead

cmyr approved these changes Sep 4, 2019

View reviewed changes

cmyr merged commit 90ee2e6 into master Sep 4, 2019

cmyr deleted the logging branch September 4, 2019 06:10

cmyr mentioned this pull request Sep 17, 2019

add option to log to stderr to AppLauncher #162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use logging crate #135

Use logging crate #135

LiHRaM commented Aug 31, 2019 •

edited

Loading

cmyr left a comment

cmyr Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

LiHRaM Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

LiHRaM Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

cmyr left a comment

cmyr Sep 3, 2019

LiHRaM Sep 3, 2019

cmyr Sep 3, 2019

cmyr Sep 3, 2019

LiHRaM Sep 3, 2019

cmyr left a comment

Use logging crate #135

Use logging crate #135

Conversation

LiHRaM commented Aug 31, 2019 • edited Loading

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

LiHRaM commented Aug 31, 2019 •

edited

Loading