Refactor IPC dispatching

[gwleuverink]

As briefly discussed in NativePHP/laravel#367

While exploring the codebase I found an existing notifyLaravel function in the electron plugins' utils.ts. It looks like this function doesn't emit events to any menubar window like was recently added to the debug endpoint. Should I create a separate issue for this?

Yes please. I think that makes sense for them all to be consistent.

When starting on this PR I only added the clause that dispatched an ipc event to the menubar window in diff

However a repeating pattern became apparent. I was seeing this in 3 spots:

Object.values(state.windows).forEach(window => {
    window.webContents.send('native-event', { event, payload })
})

if (state.activeMenuBar?.window) {
    state.activeMenuBar.window.webContents.send('native-event', { event, payload })
}

This seems like a crucial & easy to miss implementation detail. It might be good to extract that so dispatching events to the menubar isn't missed in the future.

This is only a small refactor. Please feel free to disregard or point me to any changes I can make to make this work for you.

If you prefer I can cherry-pick d11ba10 and resubmit the PR 👍🏻

[gwleuverink]

It seems some commit hashes are included that aren't present upstream. Sorry for the mess.

The only new changes are made in d11ba10 & 5fed227

[gwleuverink]

I found two ipc calls for your review. They look like they might be unused:

electron-plugin/src/server/api/window.ts

Line 192 in 7bae295

window.webContents.send('window:blur')

electron-plugin/src/server/api/window.ts

Line 200 in 7bae295

window.webContents.send('window:focus')

[simonhamp]

Nice refactor

[simonhamp]

I found two ipc calls for your review. They look like they might be unused

Agreed, i'm not sure what these are for. @mpociot can you recall why you added these?

[mpociot]

@simonhamp Yeah, the idea is to have these events available so we could use them in combination with TailwindCSS.

For example:

in my app.blade.php file:

<script>
    const electron = require('electron');
    const ipcRenderer = electron.ipcRenderer;
    ipcRenderer.on('window:blur', () => {
        document.documentElement.classList.remove('focused');
        document.documentElement.classList.add('blurred');
    });
    ipcRenderer.on('window:focus', () => {
        document.documentElement.classList.remove('blurred');
        document.documentElement.classList.add('focused');
    });
</script>

Then I can add focused and blurred variants to my Tailwind config file:

plugin(function({ addVariant, e }) {
    addVariant('blurred', ({ modifySelectors, separator }) => {
        modifySelectors(({ className }) => {
            return `.blurred .${e(`blurred${separator}${className}`)}`
        })
    })
}),
plugin(function({ addVariant, e }) {
    addVariant('focused', ({ modifySelectors, separator }) => {
        modifySelectors(({ className }) => {
            return `.focused .${e(`blurred${separator}${className}`)}`
        })
    })
})

And then use them in my layout like: class="blurred:text-gray-300" to achieve a macOS-like UI behavior when the window gets blurred.

[simonhamp]

@mpociot Right. So I think you could achieve this with the slightly more generic Native.on('Native\Laravel\Events\Windows\WindowBlurred') and Native.on('Native\Laravel\Events\Windows\WindowFocused')

@gwleuverink let's leave these shortcuts in for now tho - we can clean this up before v1

[mpociot]

Right, but all of these events ha e a slight overhead as they get passed through PHP and then broadcast again, if I'm not mistaken.

[gwleuverink]

Thanks for your feedback! I'm just getting familiar with NativePHP's inner workings so I appreciate you taking the time a lot!

From what I could gather by reading the code, all events passed through the notifyLaravel function first are passed to PHP and then dispatched over IPC, but only if the passed endpoint parameter was 'events'. See line

So I get that there is a bit extra overhead as marcel mentioned. But leaving those extra IPC calls there as is they are effectively dispatched twice, under a different event name.

Couldn't we maybe consolidate this by not awaiting the http call to the backend? The call to php is made regardless, but I don't think there is a need to wait for it if I understand correctly

[mpociot]

Hm wouldn't it be sufficient if we move the if condition / IPC dispatching before the axios call?

But we could also get rid of the await I think, as we don't really need to wait for it to finish anyway

[simonhamp]

Agreed, either approach here seems fine. I have to say I prefer to await where possible tho as it makes the code a little more explicit