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

Added the option for image to take its original size #815

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

MisustinIvan
Copy link
Contributor

you can now do .Size(0,0) and the image takes its original size

@MisustinIvan MisustinIvan changed the title Added the option for image to take it original size Added the option for image to take its original size Jul 8, 2024
@MisustinIvan
Copy link
Contributor Author

Because i am not if there is a way to do this without this change if you use image from URL. You can of course just write your own function that fetches the image and gets the size, but whats the point of ImageWithURLWidget? I was also thinking about adding a option to set a size constraint and just scale the image while keeping the aspect ratio to fit the constraint. It would be nice if you shared some feedback, thanks!

@MisustinIvan
Copy link
Contributor Author

I have also thought about replacing the behavior when passing in (-1, -1) to Size with this, because the current one kinda sucks and just stretches the Image.

Copy link
Collaborator

@gucio321 gucio321 left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for your PR.
I think this is very good idea and I'm wondering why it isn't there already 😁

Just a few things:

ImageWidgets.go Outdated Show resolved Hide resolved
ImageWidgets.go Outdated Show resolved Hide resolved
@gucio321 gucio321 added the enhancement New feature or request label Jul 9, 2024
@MisustinIvan
Copy link
Contributor Author

Made it default behavior, ran the CI on my fork, everything seems fine to me. You can test the new behavior using the provided code snippet (you have to download the image):

func loop() {
	g.SingleWindow().Layout(
		g.Style().SetFontSize(20).To(
			g.Label("Default behavior:"),
		),
		g.Label("Image with URL:"),
		g.ImageWithURL("https://go.dev/blog/gopher/header.jpg"),
		g.Label("Image with file:"),
		g.ImageWithFile("./header.png"),

		g.Dummy(0, 100),

		g.Style().SetFontSize(20).To(
			g.Label("Custom size:"),
		),
		g.Label("Image with URL:"),
		g.ImageWithURL("https://go.dev/blog/gopher/header.jpg").Size(200, 200),
		g.Label("Image with file:"),
		g.ImageWithFile("./header.png").Size(200, 200),
	)
}

func main() {
	wnd := g.NewMasterWindow("New img behavior test", 1000, 1000, 0)
	wnd.Run(loop)
}

@MisustinIvan
Copy link
Contributor Author

And would you be able to recommend me some things to work on in this project? I find it quite cool and would like to contribute some more. 😄

Copy link
Collaborator

@gucio321 gucio321 left a comment

Choose a reason for hiding this comment

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

2 more things:

  • could you wite a comment over ImageWidget (and probably ImageWithURL and other Image*type definition and describe what0` does
  • there is still one liner issue

ImageWidgets.go Outdated Show resolved Hide resolved
@gucio321
Copy link
Collaborator

@MisustinIvan generally, we have 3 types of issues:

  1. giu-related stuff - e.g. missing functions, some nice enhancements for API (or things like this PR). I've tagged them with good first issue Good for newcomers
  2. issues with cimgui-go (which are the worst sh*t I've ever seen 😄). In general they are caused by our migration from imgui-go to cimgui-go and the fact that cimgui-go was/is early in development. It works but it requires some fixes.
  3. well, we have one more type of issue: since migration in cimgui-go Migration #628 I was forced to disable some widgets (e.g. CodeEditor, MarkdownEditor end so on). Now it'd be nice to re-enable them but it requires work on cimgui-go and even writing some C wrapers for C++ projects in lua (basing on https://github.com/cimgui like I did e.g. for markdown in https://github.com/gucio321/cimmarkdown)

@MisustinIvan
Copy link
Contributor Author

Added the comments and fixed linter complaint, everything should be fine.

@MisustinIvan
Copy link
Contributor Author

Bruh does the linter really want comments to end with period...

@MisustinIvan
Copy link
Contributor Author

Check passed, everything seems fine to me!

Copy link
Collaborator

@gucio321 gucio321 left a comment

Choose a reason for hiding this comment

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

nice, thank you

@gucio321 gucio321 merged commit 0a46b90 into AllenDang:master Jul 10, 2024
4 checks passed
@gucio321
Copy link
Collaborator

Bruh does the linter really want comments to end with period...

you can use golangci-lint run --fix ./... and this should automatically fix that.

@MisustinIvan
Copy link
Contributor Author

thanks, will try that next time!

@MisustinIvan MisustinIvan deleted the image_with_url_size branch July 10, 2024 11:13
@MisustinIvan MisustinIvan restored the image_with_url_size branch July 10, 2024 11:13
@MisustinIvan
Copy link
Contributor Author

btw what is the best practice, github is asking me if I want to delete the feature branch. Should i delete it or keep it?

@gucio321
Copy link
Collaborator

I thing you should delete. these commits are in master anyway and if you keep every branch, you'll have hundreds of them some day 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants