Background modes implementation #6

Abdillah · 2017-04-10T13:33:44Z

Implement background modes following other desktop environment (Windows, MacOS, etc). Feedback is welcome, since this is not optimized yet. Plus, I'm not very well on describing neither stride nor pool.

This will change the arguments into

way-cooler-bg /path/to/image.jpg 'tile' # the type: or 'fill', 'fit', 'stretch', none

christophebiocca

Looks solid overall, I went and picked a lot of small nits.

christophebiocca · 2017-04-10T16:59:01Z

src/main.rs

+            "stretch" => BackgroundMode::Stretch,
+            "center"  => BackgroundMode::Center,
+            "tile"    => BackgroundMode::Tile,
+            _         => BackgroundMode::Fill,


This file seems to have a preference for .expect() and ::std::process::exit(1); so far.

Does it make sense to do that here as well? Return a Result<BackgroundMode> and let the caller call expect on it?

Yes, I agree with @christophebiocca, we should be returning a Result here that fails if it's not one of the options.

christophebiocca · 2017-04-10T17:00:24Z

src/main.rs

+}
+
+impl BackgroundMode {
+    fn from_str(s: &str) -> BackgroundMode {


Consider implementing FromStr here, as it is a standard trait for this kind of task.

christophebiocca · 2017-04-10T17:01:08Z

src/main.rs

@@ -80,7 +112,12 @@ fn main() {
        let color = Color::from_u32(color);
        generate_solid_background(color, &mut background_surface, &env)
    } else {
-        generate_image_background(input.as_str(), &mut background_surface, &env)
+        let mode = if args.len() >= 3 {
+            BackgroundMode::from_str(&args[2])


If you implement FromStr above, you can then use args[2].parse() here.

christophebiocca · 2017-04-10T17:03:17Z

src/main.rs

+        }
+    }
+}
+
 fn main() {
    let args: Vec<_> = env::args().collect();
    if args.len() < 1 {


Now that there's more than one argument, it might be worth it to call out to an argument parsing library, otherwise users will have to remember which order arguments go in.

As a bonus, I think at least clap (and maybe others) know how to automate the parsing of arguments that implement FromStr which means a nicer, human friendly message if the wrong type of scaling mode is passed in.

christophebiocca · 2017-04-10T17:17:35Z

src/main.rs

+
+    // Mode image processing
+    // The output must be scr_width x scr_height resolution
+    image = match mode {


Most of these reassignments could be replaced by rebinding (using let image = a second time). This is something that Rust lets you do.

So shadowing is preferred than mutable, right?

christophebiocca · 2017-04-10T17:20:37Z

src/main.rs

+        },
+        // TODO: Padding background color
+        BackgroundMode::Center  => {
+            // FIXME: How if image bigger than screen size?


To handle that case, you'd want to use sub_image.

It seems possible to combine all 4 cases (<W,<H), (>W,<H), (<W,>H), (>W, >H) by using sub_image and copy_from with the appropriate mix of max(difference,0) and max(-difference,0).

I'll try. This is interesting problem to solve..

christophebiocca · 2017-04-10T17:24:16Z

src/main.rs

+            imagepad
+        },
+        BackgroundMode::Tile    => {
+            let repeat_x_count: u32 = ((scr_width / img_width) as f64).ceil() as u32 + 1;


I think ((scr_width / img_width) as f64).ceil() as u32 doesn't do a ton here:

scr_width / img_width truncates

casting it to f64 works but it's already too late, it's truncated.

.ceil() is then guaranteed to do nothing.

I think ((scr_width as f64) / (img_width as f64)).ceil() as u32 is what you want? Then the + 1 at the end can safely be removed.

Lol to myself: It's truncated! 😆
Thanks @christophebiocca

Timidger · 2017-04-10T19:07:49Z

src/main.rs

@@ -10,6 +10,7 @@ extern crate dbus;
 use std::env;
 use std::process::exit;
 use image::{load_from_memory, open};
+use image::{GenericImage, DynamicImage, FilterType};


You might as well combine this use statement with the one above it (which is fetching from the same crate/module)

Mutable variable used on more limited scope.

Abdillah · 2017-04-14T13:35:41Z

I added many things:

Argument parser using clap library.. but oops Cargo.lock

USAGE:
    way-cooler-bg [OPTIONS] --color <HEX> --image <FILE> [--mode <MODE>]

Fix center mode
Support fill color

Any feedback and review welcome..

Timidger · 2017-04-14T16:33:52Z

Excellent, I'll take another look. If there's anything major I'll bring it up, otherwise I'll merge it in, bump the release to 0.2.0, and push to crates.io (and update the install script on the website

Timidger · 2017-04-14T16:58:43Z

Thanks again for you hard work! I'll update your listing on the contributors list in the main repo to include this.

Timidger · 2017-04-14T17:01:48Z

And thanks to @christophebiocca for pitching in and reviewing the code! Nice job catching that floating-point conversion/truncation error.

Abdillah added 4 commits April 10, 2017 19:58

Add background modes

f73d782

Implement background modes: fill, fit, stretch, tile

819e22f

Remove TODO comment

de2d122

Simplify mode argument acquisition

c9c8db0

christophebiocca reviewed Apr 10, 2017

View reviewed changes

Timidger reviewed Apr 10, 2017

View reviewed changes

Abdillah added 2 commits April 11, 2017 05:57

Implement FromStr for BackgroundMode

6dba76d

Change mut behaviour: prefer shadowing than mutable var

223fc1b

Mutable variable used on more limited scope.

Timidger mentioned this pull request Apr 12, 2017

Background now uses actual output geometry way-cooler/way-cooler#291

Merged

Abdillah added 9 commits April 12, 2017 19:57

Merge image lib use statement

5740283

Fix indentation

f79c358

Use FromStr trait str.parse() facility

50afd3b

Fix tile repeat calculation

204aa90

Move use statement to non-std group

bf9b08b

Change argument handling, now use clap library

f14fd60

Pass color arg to generate_image_background

8936e36

Fix center background mode when image bigger than screen

5cc4e9c

Add fill color support on center and fit modes

5f7f78d

Merge branch 'master' into abd-bg-mode

9889a90

Timidger approved these changes Apr 14, 2017

View reviewed changes

Timidger merged commit ed6baeb into way-cooler:master Apr 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background modes implementation #6

Background modes implementation #6

Abdillah commented Apr 10, 2017 •

edited

Loading

christophebiocca left a comment

christophebiocca Apr 10, 2017

Timidger Apr 10, 2017

christophebiocca Apr 10, 2017

christophebiocca Apr 10, 2017

christophebiocca Apr 10, 2017

christophebiocca Apr 10, 2017

Abdillah Apr 11, 2017

christophebiocca Apr 10, 2017

Abdillah Apr 11, 2017

christophebiocca Apr 10, 2017 •

edited

Loading

Abdillah Apr 11, 2017

Timidger Apr 10, 2017

Abdillah commented Apr 14, 2017 •

edited

Loading

Timidger commented Apr 14, 2017

Timidger commented Apr 14, 2017

Timidger commented Apr 14, 2017

Background modes implementation #6

Background modes implementation #6

Conversation

Abdillah commented Apr 10, 2017 • edited Loading

christophebiocca 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

christophebiocca Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abdillah commented Apr 14, 2017 • edited Loading

Timidger commented Apr 14, 2017

Timidger commented Apr 14, 2017

Timidger commented Apr 14, 2017

Abdillah commented Apr 10, 2017 •

edited

Loading

christophebiocca Apr 10, 2017 •

edited

Loading

Abdillah commented Apr 14, 2017 •

edited

Loading