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.

Sign up for GitHub

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

Jump to bottom

Display multiple images implementation, some minor fixes #307

Merged
merged 26 commits into from
Jan 22, 2019
Merged

Display multiple images implementation, some minor fixes #307

merged 26 commits into from
Jan 22, 2019

Conversation

arthmis
Copy link
Contributor

@arthmis arthmis commented Jan 20, 2019 •
edited
Loading

@arthmis
updating dependencies and features and added window module
b5b931c
@arthmis
added entire field for sdl2
e9d20ef
@arthmis
fixed some issues in cargo.toml and removed some useless code in wind…
eaaf59f 
…ow.rs
@arthmis
fixed typo in toml
7c7ea87
@arthmis
made dispay_image public
1ca16d8
@arthmis
tidied up window module and cargo.toml
9bea1e0
@arthmis
display_image can now only take positive window dimension arguments
b9ba803
@arthmis
fixed some type issues and logical errors with window dimensions in e…
cf75fef 
…vent loop
@arthmis
added cfg for feature display-window
2714ada
@arthmis
added some doc comments, code comments, and cleaned up some unnecessa…
090d561 
…ry code
@arthmis
fixed comment concerning get_output_image
a4d75f2
@arthmis
accidentally erased canvas.present(), now added it back to resize event
8dc6fc9
@arthmis
trying to simplify the code
f567d4f
@arthmis
fixing some problems
4ffa6d3
@arthmis
fixed a brackets issue
d49dbf6
@arthmis
fixed up the code
cfbd0f5
@arthmis
added suggested review fixes
9661f14
@arthmis
ran rustfmt
e67b3eb
@arthmis
mentioned minimum window size in docs
87039b2
@arthmis
adopted changes from upstream
74c81de
@arthmis
attempting to display multiple windows, currently giving signa SIGSEG…
5c3cff2 
…V (Address boundary error)
@theotherphil
Copy link
Contributor

theotherphil commented Jan 20, 2019 •
edited
Loading

Thanks! I’ll have a look through this tomorrow.

Can the body of display_image now be replaced with a call to display_images that constructs a single element array?

@arthmis
Copy link
Contributor Author

arthmis commented Jan 21, 2019

I was getting segmentation fault's sometimes when running it on ubuntu 18.04, so tell me if you experience any issues. It seems to be a problem with sdl2 because it works fine on windows.

@arthmis
Copy link
Contributor Author

arthmis commented Jan 21, 2019 •
edited
Loading

Yeah it looks like it can, I'll make the change if nothing weird crops up

Whoops, I've made a huge error and erased all of my changes with a rebase. I have to correct that. I was trying to rename the author for some commits i made lol.

@arthmis
display multiple images
e56f099 
* working on multiple image displays in a new branch

* managed to open two windows without resizing events working

* got most of the logic working, now have to figure out how to quit out application after closing all windows one by one

* trying to work with sdl2_sys window flags

* ran rustfmt, implemented multiple image displays, added empire state building jpeg
@arthmis
implemented display_image using display_multiple_images
e81301a
theotherphil
theotherphil reviewed Jan 21, 2019
View reviewed changes
Copy link
Contributor

@theotherphil theotherphil left a comment

Choose a reason for hiding this comment

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

I've left a few minor comments. I don't know much about sdl2 but this otherwise look reasonable to me!

src/window.rs Outdated Show resolved Hide resolved
src/window.rs Outdated Show resolved Hide resolved
src/window.rs Outdated Show resolved Hide resolved
src/window.rs Show resolved Hide resolved
@arthmis
Copy link
Contributor Author

arthmis commented Jan 22, 2019

If and when you merge, can you squash and merge please?

theotherphil
theotherphil reviewed Jan 22, 2019
View reviewed changes
Copy link
Contributor

@theotherphil theotherphil left a comment

Choose a reason for hiding this comment

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

I've left a couple of comments, but just noticed that you're still in the middle of changing this. Give me a ping when this is ready for review. And yes, I'll squash and merge! Thanks

examples/multiple_image_displays.rs Outdated Show resolved Hide resolved
examples/display_multiple_images.rs Outdated Show resolved Hide resolved
@arthmis
Copy link
Contributor Author

arthmis commented Jan 22, 2019

@theotherphil is this how u ping?

@theotherphil theotherphil merged commit a29b389 into image-rs:master Jan 22, 2019
@theotherphil
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theotherphil theotherphil theotherphil left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arthmis @theotherphil