Adding new options and actions. #7
Conversation
Added isCreationInProgress boolean and onFinishAction and onPageLoadAction
Added OnFinishAction and OnPageLoadAction handling and fixed a race condition when creating WebView by using the new isCreationInProgress boolean.
Added checks to prevent multiple creation attempts of WebView2 environment and controller. Removed unnecessary global meter and redraw bangs. Changed Initialized notice to debug. Added onPageLoadAction and onFinishAction.
Re-ordered creation check added check for initialization added missing reset when hr fails.
| callbackResult = result; | ||
|
|
||
| // Trigger Rainmeter redraw after callback completes | ||
| if (skin) |
There was a problem hiding this comment.
What for? I would strongly suggest to avoid triggering global meter updates and redraws from the plugin. That could lead to unwanted behavior for skin authors.
There was a problem hiding this comment.
May I ask why this was removed? This logic ensures that the skin redraws on the initial page load and that the initialization callback is executed.
There was a problem hiding this comment.
Because there’s no need to hard code it. Skin authors can manually add those bangs if and when necessary. Some skin authors (including me) prefer to have total control of when their meters update and the skin redraws, hard coding to force update all meters and redraw can cause confusion and unwanted behavior.
There was a problem hiding this comment.
This logic ensures that the skin redraws on the initial page load
If this (updating all meters and redraw the skin) is absolutely critical to correctly painting the skin, its meters, and the WebView control, then keep it. If not and that takes care on its own anyway (like it seems from the plugin version adjusted by RicardoTM, which works the same as yours in that regard after I tested it), then remove it.
Personally, I too think that users should have the freedom to control the updates / redraws as they see fit, if such actions are not esssential for having a functional code / plugin displaying correctly in the skin. Especially if the user is in fact offered the possibility of doing that, like here. If both efficiency and functionality are possible, then great!
So, I think that this revolves around having a specific case where not having all meters updated and the skin redrawn built into the plugin produced any kind of glitches / malfunction / unexpected behavior. If such a case doesn't exist, then this part of the code isn't really necessary. My two cents about it, anyway...
There was a problem hiding this comment.
At least for all tests I did, no, it doesn’t ensure anything. All demo skins load and work perfectly fine without those hard coded bangs. They are only executed if window.OnInitialize() is used (not empty).
In such case, the user can add them manually to the callback if needed (which they are not on such example):
window.OnInitialize = function() {
console.log("🚀 WebView initialized!");
RainmeterAPI.Bang('[!UpdateMeter *][!Redraw]');
return "Ready!"; // This becomes the measure's value
};
On the current plugin state, those bangs on the example above would execute twice, first the ones the user coded on the example above, then the hard coded ones.
There was a problem hiding this comment.
By the way, I can see why you may think it is important. Because without it, on the JSInteraction.ini demo skin, the plugin's string value will not be displayed on the [MeterStatus] as soon as it's ready. However, the way I added OnPageLoadAction helps doing that if necessary.
The OnPageLoadAction waits and executes after the OnInitialize function, so simply adding
OnPageLoadAction=[!UpdateMeter *][!Redraw] will function exactly the same way for your example skin, without needing to hard code that at the plugin level.
| isCreationInProgress = false; | ||
|
|
||
| if (rm) | ||
| RmLog(rm, LOG_NOTICE, L"WebView2: Initialized successfully with COM Host Objects"); |
There was a problem hiding this comment.
I think using debug is less annoying than using notice, that way you only see it when debug mode is enabled.
There was a problem hiding this comment.
Sure. I plan to change this to a Debug-level log in the final release. Right now, the default Rainmeter debug messages appear alongside ours, which makes the log harder to read and understand
There was a problem hiding this comment.
Ok, I agree to that decision 👍🏼
WebView2/WebView2.cpp
Outdated
|
|
||
| if (wcslen(onFinishAction.c_str()) > 0) | ||
| { | ||
| RmExecute(skin, onFinishAction.c_str()); |
There was a problem hiding this comment.
I'm triggering it here, but it could be moved to the Reload() function after CreateWebView2() finishes, that way It would trigger whether CreateWebView() succeeds or not.
There was a problem hiding this comment.
I am planning to add support for OnPageCreateAction and OnPageReloadAction
There was a problem hiding this comment.
Here the OnFinishAction has nothing to do with the page state, it is more about when WebView has finished being created (or re-created).
It helps triggering an action when the measure number value goes from 0 to 1. It also allows to self update the measure (manually) when WebView2 is ready.
For example, when Update=-1 one can do:
[Rainmeter]
Update=-1
[WebView]
…
OnFinishAction=[!UpdateMeasure #CURRENTSECTION#][!Log “WebView2 has been initalized”]
However, it will only trigger if webview is created successfully, so it can either be left like that, and add an additional OnErrorAction which would trigger if WebView is not initialized for whatever reason (like missing webview2 runtime). Or, as I mentioned, it can be moved so OnFinishAction triggers either WebView initializes or not (I would recommend the first option for better control).
The other one I added is OnPageLoadAction. That one triggers every time the page loads, either first load or on reload. That way we can trigger the same bangs on every page load.
i’m ok with adding OnPageCreateAction, however not sure what would that be useful for.
OnPageReloadAction I think is unnecessary, when having OnPageLoadAction but I guess it doesn’t hurt to add.
I guess OnPageCreateAction would be triggered only on the page’s first load (I would suggest naming it OnFirstPageLoadAction instead if that is the case), OnPageReloadAction on the following reloads, and OnPageLoadAction on every load.
Those 3 would bring enough control for loading indeed.
|
Thanks for this PR. However, I have some concerns about it. |
Removed redundant redraw calls after callback completion. This was causing CallJS to execute twice..
| { | ||
| RmExecute(measure->skin, L"!UpdateMeter *"); | ||
| RmExecute(measure->skin, L"!Redraw"); | ||
| } |
There was a problem hiding this comment.
Again, we need to avoid these unnecessary global updates.
This was causing CallJS to execute twice when called. See JSInteraction.ini, it's being logged twice. Removing this fixed it.
Note: The 1 update delay on the result on that demo skin may be unavoidable. Other ways to fix that may be investigated anyway. However, I find it unlikely due to the calls being async, so we may have to live with it (not a big deal anyways).
We could try doing it synchronously instead, unfortunately WebView2 doesn't offer a synchronous ExecuteScript function, however, someone offered a solution that we could try. Yep, we would risk blocking rainmeter for heavy scripts, but I think lua does block rainmeter too when scripts are heavy, so it may not be such a big deal. We would do it synchronously only for ExecuteScript calls by the way.
Here’s another, pretty long conversation about the same topic.
Notes on Global Updates and JS InteractionI agree that avoiding global updates is necessary, but this leads to several issues:
You can test all of these behaviors using the following file: How to Test
I want to see is it happening or solved. |
|
No, it’s not fixed. That can’t be fixed unless we somehow call ExecuteScript synchronously, which is not possible out of the box. See my note here: #7 (comment) Not even your workaround fixes it. You thought it was fixed because it was redrawing the whole skin twice, which “fakes” a solution, but if you look at the actual string value of the measure, it is still delayed. Your workaround was actually causing a meter update and redraw loophole. |
Ok, here's my attempt to solve the sync issue. Unfortunately, all my attempts to add a similar solution for window.OnInitialize and CallJS failed, both create a deadlock when implemented this way. However, this appears to solve the issue on window.OnUpdate which I think is the most important. Please build my Working2 branch and test it, you should see now both Status: Update #N on Rainmeter and on WebView are correctly synced. As well as the Measure's string value. Also add OnPageLoadAction=[!UpdateMeter *][!Redraw] on the WebView measure to correctly see the "Initialized!" message on the status string.
There was a problem hiding this comment.
Ok, here's my attempt to solve the sync issue.
Unfortunately, all my attempts to add a similar solution for window.OnInitialize and CallJS failed, both create a deadlock when implemented this way.
However, this appears to solve the issue on window.OnUpdate which I think is the most important.
Please build my Working2 branch and test it, you should see now both Status: Update #N on Rainmeter and on WebView are correctly synced. As well as the Measure's string value.
Also add
OnPageLoadAction=[!UpdateMeter *][!Redraw]
on the WebView measure to correctly see the "Initialized!" message on the status string.
Note: It is very prompt to crash Rainmeter, so I don't really see it as a solution but as an experiment. My honest solution would be to actually live with the plugin having a delayed return, which would avoid a lot of headaches😅. I think any attempt of a solution will be pretty hacky and fragile.
Did you try it yourself? Is it crashing Rainmeter or not? |
I'd be glad to help in finding a solution to this, but I just can't understand what that file is supposed to do. Display 15 + 25 = 40 over and over again? How should I compare the behaviors in the two cases if all it does is return that sum, in the skin or the wbeview console? Sorry for being obtuse on this, but I don't get what I should look for or what the expected result would have to be. Other than that, just a different perspective: if the WebView2 environment always works with async calls, why bother making it sync from the WebView2 side, and not put Rainmeter or the skin on hold until the WebView2 result is completed instead? If the mountain won't come to Muhammad, let Muhammad come to the mountain - no offense, it's just a saying that is widely used where I live, and captures the fact that there are always at least two ways to approach a problem, depending on which has the most chances to work or be implemented. Another possibility would be to trigger the JS somewhat sooner so it has the time to return by the time the result should be displayed. Of course, messing with the updates in the skin or estimating the time needed for an operation to complete isn't exactly ideal, but well, if it can't be done otherwise... |
Here's a more natural way to do things... JSInteraction.ini: The only thing I notice in both your 0.0.6 version of the plugin and the one in which RicardoTM added the OnPageLoadAction, is that occasionally computations are displayed twice in both the skin's WebView and the WebView console. The "Initialized!" message on the status string is not typically displayed (maybe it's replaced with the "Update #N" one too fast, or not displayed separately so it can be seen directly?), unless I do Ctrl +F5 to reload the page repeatedly and hope to see it. And that is with RicardoTM's version of the plugin, by the way (in which I think he removed the discussed couple of lines from the plugin code, if I understood this correctly). P.S. After a while, the "Result from JS:" value is displayed as "0". Not sure why, and didn't bother investigating it. |
Of course I tried it myself. It does crash Rainmeter if you try to interact with it while it’s loading. I’ll build it later and post it here just for the record. I will undo the changes afterwards. [Deleted] |
No, on the last version I sent you that’s not fixed yet. To be clear. The issue is that the measure’s string value is not properly in sync with the returned value from JS, it’s delayed by one update cycle. The other issue is that the fix nstechbytes tried forces the skin to update all meters and redraw on every update, which causes the status meter to “sync” but it also causes it to draw twice on every update. The fix I talked about earlier works but makes Rainmeter crash if you interact with the skin while WebView is loading. Making the skin wait sounds like a solution but that’s not possible, because we are talking about the string value of the measure. To be precise: Rainmeter asks the plugin to update, the update function on the plugin tells WebView to execute the JS’s window.OnUpdate function asynchronously. At this point, Rainmeter already returned the old value because it can’t wait until JS returns a value. So that value is returned until the next update. The same happens to the window.OnInitialize and the CallJS functions. You can see it by comparing the value on the WebView window with the string value of the measure. While JS displays Update 5 the string value is returning Update 4. There’s no way to execute the JS functions “sooner”, they are executed as soon as their plugin counterparts are executed, I mean, you can’t command JS to update before commanding the skin to update first. I’ll build and send you the version without nstechbytes solutions for you to see it more clearly Edit. Sent. |
Removed synchronous script execution function and adjusted script execution logic in Update function.
Ah, ok, I now see it. Phrasing things like "the [MeasureWebView] string value is different than the value displayed by Update #N in the skin" would have properly identified / named these things and helped in understanding it better - just saying, from my own perspective. There could be other measures in the skin, and other values returned from JS, and not identifying them specifically makes things unclear. Somewhat similar issue on the forum when folks don't name the skin they have issues with. ;) That being said, I forgot to close the JSInteraction skin I posted before (along with the Rainmeter log window and the WebView console in DevTools) for a couple of hours, and when I came back the laptop's fans were going crazy - see the usages below: P.S. I was using the same plugin version you (RicardoTM) sent me, if that matters. Will test the version you sent me now later this evening. EDIT: Unfortunately, it didn't occur to me to check it at the time I closed things, but I tend to think that this high usage might have been caused by the huge console list of all the printed values. I might be wrong though, not sure, as I didn't look closely if the usage got back to normal after closing the DevTools window... |
I'm pretty sure that was the cause. I've let it running for a while and I don't have any issue. Logs consume memory and cpu for sure. |
|
Ricardo, do you have any plans to implement OnPageReloadAction? |
I do. The ones I already have working are: I have planned: All names are subject to change, ideas are welcomed. Ideas on other actions are also welcomed. I also have a new option working called |
|
I have finished implementing:
Will commit new changes soon. |
Thanks! After this, we will test all the changes and merge them into the branch. |
Renamed `OnFinishAction` to `OnWebViewLoadAction` Added actions: `OnWebViewFailAction` `OnPageFirstLoadAction` `OnPageReloadAction` Added `raincontext` and `isFirstLoad` booleans. Added `UpdateRaincontext()` function.
|
Ok, I'm going to elaborate on the issue I'm having so that way you can give ideas. The main problem is that WebView has no way to detect what a reload is. So, I tried 2 main ways: The first was based on user commands, which is not a good fit because there are many special cases were reloads are falsely detected/undetected. The second one and more promising was comparing the previous URL to the new URL, however, there's an issue. So what I tried next was to ignore all query parameters and base the decision on the new normalized URL. So I'm kind of stuck. I'm not a web expert, so maybe there is a list of queries that we can safely ignore, like the google ones. However if such a list exist, I believe it would be too extensive to implement. Ideas? Edit. The same applies to |
|
The following WebView2 implementation helps reliably detect when a page is reloaded versus when a new navigation occurs. This method compares the current navigation URL with the previously completed navigation URL:
Code// Track the last successfully loaded URL
std::wstring currentUrl;
webView->add_NavigationStarting(
Callback<ICoreWebView2NavigationStartingEventHandler>(
[¤tUrl](ICoreWebView2* sender, ICoreWebView2NavigationStartingEventArgs* args) -> HRESULT
{
wil::unique_cotaskmem_string uri;
args->get_Uri(&uri);
if (uri.get())
{
if (std::wstring(uri.get()) == currentUrl)
{
OutputDebugStringW(L"Reload detected!\n");
}
else
{
OutputDebugStringW(L"New navigation\n");
}
}
return S_OK;
}).Get(),
&token);
webView->add_NavigationCompleted(
Callback<ICoreWebView2NavigationCompletedEventHandler>(
[¤tUrl](ICoreWebView2* sender, ICoreWebView2NavigationCompletedEventArgs* args) -> HRESULT
{
wil::unique_cotaskmem_string source;
sender->get_Source(&source);
if (source.get())
{
currentUrl = source.get();
}
return S_OK;
}).Get() |
Did you test it? That’s exactly what I’m already doing. Now do the test that I mentioned on my last post. Go to google.com then reload the page, it won’t detect it as a reload because the URL changes every time you reload. |
Sure, I’ll test it soon. |
|
Well, I'm out for tonight. Here's the latest test build: [Deleted] Changes: Actions
On WebView there are 3 navigation steps:
Please suggest the final names for at least these 3 actions, as I think all 3 will stay. When navigation finishes I added a couple debug logs that print when the URL changes, and also print the current URL. I forgot to change them to Notice, and I'm already tired to do that, so make sure you have Debug mode enabled to see them. Just as an experiment, now the plugin's measure returns the current URL. The measure will also self-update when the URL changes to show the current URL as it changes even when Now Yincognito’s script won’t be injected if raincontext option is disabled. Here's the BangCommand.iniTo reproduce the issue with
What's the issue? PS. Sorry, I haven't changed the name of Raincontext. I'm still waiting for NSTechBytes to agree with the changes. |
|
I think all other actions are good. The Let's see what happens and try to solve the reload issue. |
Agreed as well, after testing the attached package - it all looks good, the names are fine, just the That being said, the Google query parameters for the session issue does happen for me too occasionally, it's been around a month since I first noticed it (obviously didn't like it) and found what it looks like an explanation for it, see here. By the way, a different query in the URL usually leads to a different page / result, see the queries to get the weather.com's JSON - so I wouldn't worry too much about the specific google.com issue - let them sort it out (yeah, right). Other than the minor Google issue, everything works fine for me - great work, both of you! Thank you both for all the time, effort and patience on this. Grateful for it, Ricardo - for sure you'll put these to good use in your own WebView2 skin / skins too! EDIT: For the record, I don't think it was the plugin, but on the very first load of the skin / plugin, I got this (notice the blank): |
|
Alright, thanks for the link. It only happens to me on Edge, on Opera is all good. I’ll try the fix later. I’ll change Raincontext to AllowDualControl and will remove the URL debugs. And change the other to notice. I’ll commit afterwards. Well, not sure why you have those issues, maybe your Laptop is finally giving up :( |
Any reason why? I kind of liked it... unless nstechbytes has other idea for the numeric and string value of the measure, I think they should return something useful (in the lack of a better choice, the URL seemed like a good choice for the string value, since you might not always know it beforehand, e.g. the google case; the meaning of the number value should be mentioned in the docs too, by the way). My opinion, at least. |
Because it was just for testing and it's out of the scope of this PR. Also it would crash with the My take on it is: But yeah, I do prefer the state machine option than letting the user return a delayed value. But we will discuss that on another PR to stay organized. |
Ah, ok then, you're right - I didn't think of that. I was just wondering if the user couldn't use the string value himself to handle the URL query strings like on google, as desired. Plus other situations where the URL was a computed value and you'd need to know the "real" / "eventual" URL that was loaded. Might be useful for cloudflare protected sites too, or redirections. But I understand what you're saying, and I don't want to abuse you in any way, you've done a lot until now. Up for you guys to decide on that whenever it's possible, you already know my opinion about it (from a skin designer perspective). |
Added new actions.
Raincontext is now called AllowDualControl. The script is no longer injected when the option is disabled. Added new actions.
Added new event listeners for triggering new actions.
|
This PR is now marked as ready. SUMMARY OF CHANGES Actions Added
Navigation state actions:
Options Added
Other Changes
Known Issues
Build for testing Skin for testing BangCommand.ini |
|
Sorry, added ZoomFactor to please Rooky hehe. I'll update the previous post with most recent build and skin. Edit: Done. |
Good idea, in my view. Now you'd have to add an P.S. Just joking on the last part - take a good rest, Ricardo, you deserve it! |
shhhh! don't let him know that's possible! |
|
Hey @NSTechBytes, just to let you know I created a new branch with only 3 commits, if you prefer to close this one and merge that one let me know. It has the same code as this, only all on a single commit per file. |
Sure, I will. First, I want to test these new changes. I will merge this PR soon. |
|
One Question
It means -1,1 |
No, any value less or equal to 0 is false, and any positive value is true. Any negative value, or 0, will be |
3 participants
Added OnFinishAction and OnPageLoadAction
Fixed a race condition when creating webview2.
See commits for more info on what changed.
Ps.
OnFinishAction and OnPageLoadAction names are not final and can be changed if you want.
OnFinishAction is triggered when CreateWebView2 finishes.
OnPageLoadAction is triggered whenever the current page finishes loading.