Skip to content

Addition of a bar displaying the name of the image being displayed#3

Open
AlexCreamer wants to merge 10 commits into
pedrocr:masterfrom
AlexCreamer:information_bar
Open

Addition of a bar displaying the name of the image being displayed#3
AlexCreamer wants to merge 10 commits into
pedrocr:masterfrom
AlexCreamer:information_bar

Conversation

@AlexCreamer

Copy link
Copy Markdown

No description provided.

@pedrocr pedrocr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Besides the smaller stuff I've pointed out as we've discussed on IRC it would make sense to show the image width and height as well at least.

Comment thread src/main.rs Outdated
.center_justify()
.line_spacing(10.0)
.set(ids.lefttext, ui);
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Whitespace here is misaligned

Comment thread src/main.rs Outdated

if let Some(ref f) = file {
let file_name = Path::new(f).file_name().unwrap();
widget::Text::new(file_name.to_str().unwrap())

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These unwrap()s need to be handled

Comment thread src/main.rs Outdated
(ids.imgcanvas, widget::Canvas::new().color(color::CHARCOAL).border(0.0)),
(ids.leftarea, widget::Canvas::new().color(color::CHARCOAL).border(0.0).flow_down(&[
(ids.imgcanvas, widget::Canvas::new().color(color::GREY).border(0.0)),
(ids.footer, widget::Canvas::new().color(color::CHARCOAL).length(75.0).border(0.0)),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The footer is probably too tall for just the text. It should be sized to hold 1.5 font heights or something like that.

Comment thread src/main.rs

// The `WidgetId` for our background and `Image` widgets.
widget_ids!(struct Ids { background, imgcanvas, dragcanvas, setcanvas, settop, setcont, raw_image, chimper, filenav });
widget_ids!(struct Ids { background, imgcanvas, dragcanvas, leftarea, lefttext, footer,setcanvas, settop, setcont, raw_image, chimper, filenav });

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

lefttext doesn't exist

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

lefttext is being used on line 188

Comment thread src/main.rs
.set(ids.chimper, ui);
}

const PAD: Scalar = 20.0;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is just copied from the example and has no meaning in this program. A proper variable name should be used instead.

Comment thread src/main.rs
.line_spacing(10.0)
.set(ids.lefttext, ui);
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is probably excess white space.

@pedrocr pedrocr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is still very far from being finished, sorry.

Comment thread src/cache.rs
}
}

pub fn get_image_dimensions<'a>(&'a self, path: String, size: usize) -> Option<(usize,usize)>{

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Parsing the image all over again just to get the dimensions is just broken.

Comment thread src/cache.rs
},
};
let imgwidth = decoded.width;
let imgheight = decoded.height;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This won't return the image dimensions, it will return the decoded dimensions that in the case of raws has been downsized.

Comment thread src/main.rs
(ids.imgcanvas, widget::Canvas::new().color(color::CHARCOAL).border(0.0)),
(ids.leftarea, widget::Canvas::new().color(color::CHARCOAL).border(0.0).flow_down(&[
(ids.imgcanvas, widget::Canvas::new().color(color::GREY).border(0.0)),
(ids.footer, widget::Canvas::new().color(color::CHARCOAL).length(60.0).border(0.0)),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This still needs to be sized to the font size.

Comment thread src/main.rs
let height_usize = dimensions.1;

let width = &width_usize.to_string();
let height = &height_usize.to_string();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This isn't needed at all. format! can handle the usize directly.

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