Added basic icon support for webview #24

Ouri028 · 2023-09-13T17:53:18Z

This pull request will add basic support for changing the default icon on webview.

This was only tested on a Windows machine as I do not have Linux installed.

One thing I do think we will need to do is wrap the C functions in a macro, so that they will only be exported if the OS is Windows.
This will avoid undefined errors for Linux users. Any suggestions would be appreciated.

[if windows]
fn C.set_window_icon(application_title &wchar.Character, ico_file_path &wchar.Character)

Currently this is only supported by Windows, but Linux and MacOS will come in the future.

Here is an example of how different webview windows can have different icons.

The one on the left has a darker icon compared to the one on the right.

In order to make this as abstract as possible, I had to create a new parent struct WebviewContext which takes the webview context as well as the title, this is also a design change since it would be nice to offer more meta data to developers in order to make their application as unique as possible.

pub struct WebviewContext {
pub:
	ctx &C.webview_t
pub mut:
	title string
}

Also when creating a webview window a default title and icon is set.

pub fn create(opts CreateOptions) &WebviewContext {
	ctx := C.webview_create(int(opts.debug), opts.window)
	mut s := &WebviewContext{
		ctx: ctx
		title: 'webview'
	}
	s.set_title(s.title)
	s.set_window_icon('${@VMODROOT}/assets/icon.ico')
	return s
}

I also changed the location of the test files in order to keep them within the same module as to avoid V trying to use .vmodules instead.

Basic usage:

	mut w := create(debug: false)
	w.set_title('My Awesome App!')
	w.set_size(600, 400, .@none)
	w.set_html('<html style="background: #1B2845; color: #eee">
<samp>${@FILE}</samp>
<h2>Testing set_icon</h2>
</html>')
	w.set_window_icon('my_awesome_icon.ico')
	w.run()

	w.destroy()

…d on Windows, but Linux and MacOS will be coming soon!

ttytm

Thanks for the PR, in it's current state it's unlikely to be merged so it would need some honing.

Blockers:

It introduces a major breaking change with changing the default struct from Webview to WebviewContext. We could probably rename the current Webview struct to Context and use Webview without introducing this break.
Related to this, it is probably not necessary to add the struct so that the window title can be used for set_window_icon. Instead, using get_window to get a native window pointer and using the pointer instead of the title, can be a safer and cleaner implementation. That would also fit with the comment you added.
The quality of the default icon.ico is not as good as the quality of icon_2.ico used for the test. I think the other way around would be okay, or if both icons have good quality. The link you shared also provides a good quality icon of the default V logo: https://icon-icons.com/icon/file-type-vlang/130087. Why not use this?
The set_window_icon function in lib.c.v is declared with a bool return, but the actual function in icon.c is a void function. As it can error, we can make it return a bool and use it for a proper V result return in the V wrapper function.
It's not planned to change the projects structure. So the integration tests should remain as external tests in the ./tests directory, although this currently has the trade-of that the changes need to be made in the webview directory that is in .vmodules.

Nice to haves:

Since the function name of set_title is not set_window_title we could also use set_icon as function name.
Maybe a [if windows] attribute would fit for the function as long as it just supports windows 🤔
K&R would be preferred over Allman formatting.

ttytm · 2023-09-13T19:30:33Z

[if windows]
fn C.set_window_icon(application_title &wchar.Character, ico_file_path &wchar.Character)

Having written the review from the code view I missed that you also suggested this. So yes, I think it's good as long as just windows is supported!

Ouri028 · 2023-09-13T19:33:37Z

[if windows]
fn C.set_window_icon(application_title &wchar.Character, ico_file_path &wchar.Character)
Having written the review from the code view I missed that you also suggested this. So yes, I think it's good as long as just windows is supported!

Thanks for the great feedback, I did see some changes that, I forgot to remove, like the bool 😄 .

I have added the macros, will just adjust the code and we can go for another round 😎 .

ttytm · 2023-09-13T19:56:33Z

You'll probably run into an error an error when adding the macro and trying to use a return value.

E.g:

pub fn (w &Webview) set_icon(icon_file_path string) ! {
	if !C.set_window_icon(wchar.from_string(w.title), wchar.from_string(icon_file_path)) {
		return error('Failed setting icon.')
	}
}

A V result return could be more valuable than having the temporary [if windows] macro. It would be okay then to just mention in the doc comment that only windows is supported for now. What do you think on this?

There is probably also a better option than using a bool, as it can return multiple errors. Then both errors MessageBoxW(NULL, L"Failed to load icon from file!", L"Error", MB_ICONERROR); and MessageBoxW(NULL, L"Failed to find the application window!", L"Error", MB_ICONERROR); can be signaled to the user as V errors.

Ouri028 · 2023-09-13T20:01:08Z

You'll probably run into an error an error when adding the macro and trying to use a return value.

E.g:
pub fn (w &Webview) set_icon(icon_file_path string) ! {
	if !C.set_window_icon(wchar.from_string(w.title), wchar.from_string(icon_file_path)) {
		return error('Failed setting icon.')
	}
}
A V result return could be more valuable than having the temporary [if windows] macro. It would be okay then to just mention in the doc comment that only windows is supported for now. What do you think on this?

There is probably also a better option than using a bool, as it can return multiple errors. Then both errors MessageBoxW(NULL, L"Failed to load icon from file!", L"Error", MB_ICONERROR); and MessageBoxW(NULL, L"Failed to find the application window!", L"Error", MB_ICONERROR); can be signaled to the user as V errors.

I like it, will have it return the errors instead and we can panic (that is, if we want to panic on error) when trying to set the icon on linux/macos.

I am also currently working on Linux support, just getting my dev environment up, but for this pull request, it will only be windows.

I will update the docs as mentioned.

UPDATE:

After going over the code again, I don't think it will be possible without the macros, currently I am including the header file which contains <windows.h>, if you try to build it on Linux without mingw, it will fail, any suggestions on how to handle this?

#flag linux -DWEBVIEW_GTK -lstdc++
#flag darwin -DWEBVIEW_COCOA -framework WebKit -stdlib=libc++ -lstdc++
#flag windows -DWEBVIEW_EDGE -static -ladvapi32 -lole32 -lshell32 -lshlwapi -luser32 -lversion -lstdc++

$if windows {
	#flag -I @VMODROOT/src/icon.h
	#flag @VMODROOT/src/icon.c
}

#flag @VMODROOT/src/webview.o
#include "@VMODROOT/src/webview.h"
$if windows {
	#include "@VMODROOT/src/icon.h"
}
$if linux {
	#pkgconfig gtk+-3.0
	#pkgconfig webkit2gtk-4.0
}

ttytm · 2023-09-13T20:11:25Z

Thanks for the updates.
I'll walk the dog, and come back to the PR later.

I am also currently working on Linux support, just getting my dev environment up, but for this pull request, it will only be windows.

Nice!

Removed WebviewContext. Added new V logo. Added errors to icon.c.

ttytm · 2023-09-14T01:07:11Z

I don't want to interfere with anything ongoing, let me know when this is ready then I go about it as soon as a free moment opens up 👍

Ouri028 · 2023-09-14T11:18:03Z

Sorry just pushed some extra stuff , before I went to bed.

Will let you know once I am done.

I also think, we should postpone this pull request until, I am done with the Linux version as to not use macros.

Ouri028 · 2023-09-14T15:07:06Z

So, I got it to work on Linux running Lubuntu with GTK.

MacOS will be a bit of a challenge since, I don't own one, but for now this should be good for most use cases.

What do you think?

ttytm · 2023-09-14T15:40:05Z

So, I got it to work on Linux running Lubuntu with GTK.

MacOS will be a bit of a challenge since, I don't own one, but for now this should be good for most use cases.

What do you think?

I think that's awesome @Ouri028! It's planned to publish a new version with the latest changes soon, maybe we can make the feature be part of it.

I think it's okay if mac follows later (it would be great, as incorporating communication with native mac windows might also be a good step towards solving #6 directly from this V wrapper).

There are also solutions to the platfrom related problem. I'll do some tests later so I can speak having tested this specific case without doing too much guessing.

Other things are:

I haven't fully thought through the default icon yesterday as it was quite crowded, sorry about this. Setting a default window icon is not a choice the lib should make for the user. But the feature itself is great! Adding and setting the icon in the lib can can be done for the app example as examples/v-js-interop-app/icon.ico examples/project-structure/assets/icon.ico.
Same goes for a default window title, not a choice the lib wants to make for the user.

ttytm · 2023-09-14T16:28:44Z

Platform support works. I push the changes here so it can remain part of the PR and we have some actual code to discuss about.

Ouri028 · 2023-09-14T16:34:29Z

Platform support works. I push the changes here so it can remain part of the PR and we have some actual code to discuss about.

I agree fully about it being a user choice instead of making it a default option, I will just add an example to the project such as how to set the default icon and title.

I will clean up the code and move the tests back and push the feature, I will have a look at the MacOS implementation to see what I can do there, even if just to get a sample started.

Updated tests to include icon setting. Updated examples.

ttytm · 2023-09-14T16:45:51Z

Okay, I don't want to break anything ongoing or make merging changes and suggestions to complicated. So I pushed it as temporary branch for now: https://github.com/ttytm/webview/commits/temp/add-icon-support

ttytm · 2023-09-14T16:46:52Z

As the icon_test is more of an example, the test/icon_test is obsolete I think. When it's set in the examples, it covers the same thing there. The examples also build in CI so if set_icon fails there this part is covered. This also saves adding too many icon file duplicates to the git repository.

ttytm

LGTM. Needs a rebase and addressing the latest feedback about the icon_test file. When there is no conflict with macOS I say it can be merged :) else we can use the e.g. icon_windows.v approach to reduce it to supported platforms.

Ouri028 · 2023-09-14T17:06:53Z

LGTM. Needs a rebase and addressing the latest feedback about the icon_test file. When there is no conflict with macOS I say it can be merged :) else we can use the e.g. icon_windows.v approach to reduce it to supported platforms.

I'm fine with either, I'll look into the MacOS support and see what can be done there, but if people want the feature, we can make it available as suggested icon_windows.v

ttytm · 2023-09-14T17:16:20Z

LGTM. Needs a rebase and addressing the latest feedback about the icon_test file. When there is no conflict with macOS I say it can be merged :) else we can use the e.g. icon_windows.v approach to reduce it to supported platforms.

I'm fine with either, I'll look into the MacOS support and see what can be done there, but if people want the feature, we can make it available as suggested icon_windows.v

I would also just use what works, both ways are fine. With the current way, making an else if linux conditon instated of a plain else should also work to make mac build. https://github.com/ttytm/webview/actions/runs/6188659706/job/16801211480?pr=24

Removing icon_test.v will also help not getting stuck in the linux/test CI indefinitely.

… into add_icon_support

ttytm · 2023-09-14T18:13:16Z

Workflows should run automatically after the first approval. Sorry about the delays. I'll check what is goin on there after the PR was merged. I'm back at my desk in about 2-3h. Thanks already for all the efforts 💪

Ouri028 · 2023-09-14T18:26:58Z

Workflows should run automatically after the first approval. Sorry about the delays. I'll check what is goin on there after the PR was merged. I'm back at my desk in about 2-3h. Thanks already for all the efforts 💪

Glad I was able to help, I'm actually planning on using this library to build my future desktop apps , so its mutually beneficial 😎

ttytm · 2023-09-14T22:33:19Z

Glad I was able to help, I'm actually planning on using this library to build my future desktop apps , so its mutually beneficial 😎

That's nice to hear! It's also my first choice and I'll continue to use it for my GUI Apps too. It gets better and better 😇

Now this is also finished. Good work @Ouri028!

Added basic icon support for webview, currently this is only supporte…

e1f0121

…d on Windows, but Linux and MacOS will be coming soon!

ttytm requested changes Sep 13, 2023

View reviewed changes

Now using voidptr instead of title to get application window.

881be7e

Removed WebviewContext. Added new V logo. Added errors to icon.c.

Ouri028 changed the title ~~Added basic icon support for webview (Windows Only!)~~ Added basic icon support for webview Sep 14, 2023

Added support to change the default icon for Linux using GTK.

6517af2

Updated tests to include icon setting. Updated examples.

ttytm approved these changes Sep 14, 2023

View reviewed changes

Ouri028 and others added 2 commits September 14, 2023 19:03

Fixed merge conflicts

1b57854

Merge branch 'main' into add_icon_support

7bd73e5

Ouri028 added 4 commits September 14, 2023 19:30

Removed icon test and also fixed MacOS build failing

82c00a9

Merge branch 'add_icon_support' of https://github.com/Ouri028/webview-v…

ae3b7da

… into add_icon_support

v fmt

fd3ba51

Forgot to update header file

52dc854

Fix compiling with -cstrict

5e8a286

ttytm added 4 commits September 15, 2023 00:18

Update doc comments; update include organization

0398e19

Increase test timeout len

f963605

Make updated doc for linux comment easier to follow

2626c48

Propagate set_icon errors as shorter panic variant

4a08aa0

ttytm merged commit 9e4cb3f into ttytm:main Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added basic icon support for webview #24

Added basic icon support for webview #24

Ouri028 commented Sep 13, 2023 •

edited

Loading

ttytm left a comment •

edited

Loading

ttytm commented Sep 13, 2023

Ouri028 commented Sep 13, 2023

ttytm commented Sep 13, 2023

Ouri028 commented Sep 13, 2023 •

edited

Loading

ttytm commented Sep 13, 2023

ttytm commented Sep 14, 2023

Ouri028 commented Sep 14, 2023

Ouri028 commented Sep 14, 2023 •

edited

Loading

ttytm commented Sep 14, 2023

ttytm commented Sep 14, 2023

Ouri028 commented Sep 14, 2023

ttytm commented Sep 14, 2023

ttytm commented Sep 14, 2023 •

edited

Loading

ttytm left a comment

Ouri028 commented Sep 14, 2023

ttytm commented Sep 14, 2023 •

edited

Loading

ttytm commented Sep 14, 2023

Ouri028 commented Sep 14, 2023

ttytm commented Sep 14, 2023

Added basic icon support for webview #24

Added basic icon support for webview #24

Conversation

Ouri028 commented Sep 13, 2023 • edited Loading

Basic usage:

ttytm left a comment • edited Loading

Choose a reason for hiding this comment

ttytm commented Sep 13, 2023

Ouri028 commented Sep 13, 2023

ttytm commented Sep 13, 2023

Ouri028 commented Sep 13, 2023 • edited Loading

ttytm commented Sep 13, 2023

ttytm commented Sep 14, 2023

Ouri028 commented Sep 14, 2023

Ouri028 commented Sep 14, 2023 • edited Loading

ttytm commented Sep 14, 2023

ttytm commented Sep 14, 2023

Ouri028 commented Sep 14, 2023

ttytm commented Sep 14, 2023

ttytm commented Sep 14, 2023 • edited Loading

ttytm left a comment

Choose a reason for hiding this comment

Ouri028 commented Sep 14, 2023

ttytm commented Sep 14, 2023 • edited Loading

ttytm commented Sep 14, 2023

Ouri028 commented Sep 14, 2023

ttytm commented Sep 14, 2023

Ouri028 commented Sep 13, 2023 •

edited

Loading

ttytm left a comment •

edited

Loading

Ouri028 commented Sep 13, 2023 •

edited

Loading

Ouri028 commented Sep 14, 2023 •

edited

Loading

ttytm commented Sep 14, 2023 •

edited

Loading

ttytm commented Sep 14, 2023 •

edited

Loading