Ideally, stop continuous polling about chrome-google-keep-full-screen HOT 17 CLOSED

Beta 1.2.1 is ready for anyone that has time to help test - https://cmp.onl/tjuk
@MartinLichtblau @nickfaughey @pagro

It aims to address:

• #5 (this issue)
• #6
• #7

Thanks for your help!

from chrome-google-keep-full-screen.

Upps, the extensions isn't causing these symptoms, it must be Chrome or Keep itself. Yet, continously polling isn't exactly best practice ‒ I don't wanna waste power and kill my battery for nothing.

from chrome-google-keep-full-screen.

100% agreed - I will keep this open and will switch it over to an event or mutation observer at some point, unless someone has time to do a PR before I get to it :)

Thanks for the issue report!

from chrome-google-keep-full-screen.

Better use a DOM query selector and run it every x-seconds and trigger it on chrome.tabs.onUpdated(). This way you use less resources and don't risk to run it forever (since keep notes don't load occasionally).
... Let's say I'll make a PR.

from chrome-google-keep-full-screen.

The tricky part is detecting when Keep websites changes, since it's a Single-Page-Application (SPA).
I've dealt with that issue already like this:

background.js

var matchUrl = 'keep.google.com';
chrome.tabs.onUpdated.addListener(function(tabId, changeInfo, tab) {
    if (matchURL matches changeInfo.url) {
        chrome.tabs.sendMessage(tabId, 'url-update');
    }
});

content.js

chrome.runtime.onMessage.addListener(function(msg, sender, sendResponse) {
    if (msg === 'url-update') {
        doSomething();
    }
}); 

Source: Various ways to detect DOM changes

Would you agree to (i) include a background script and (ii) to add "tabs" permission?
If so, I contribute the changes.

from chrome-google-keep-full-screen.

Wouldn't a mutation observer be simpler? I'm not opposed to your solution per se, but I would like to avoid adding permissions unless absolutely necessary.

from chrome-google-keep-full-screen.

Limiting permissions is important indeed. Why I don't favor MutationObserver is that (i) Keep has so many DOM changes until the note is loaded that it takes a huge toll on performance (ii) a Note may not load and (iii) changing something like fullscreen is a DOM change itself and thus you have to check for that or it loops forever ever faster until it crashes. So I didn't like working with it in the past and thus used the above approach. But I fully agree with you that Mutation Observer is indeed just made for that. So Mutation Observer might be the way to go, since it doesn't require more permissions.
So, if you know how it works, show me how it's done!

from chrome-google-keep-full-screen.

Thanks, Martin! I really appreciate your notes and ideas, and the time you took to give them!

I'm not familiar enough with mutation observers "under the hood" to respond intelligently, but this is something I want to look into - so I'll get back to you after I do some more reading and maybe some quick tests!

from chrome-google-keep-full-screen.

Hey, Martin,

So, overall, I like your idea of watching for the URL change the best - the only downside is that it doesn't handle new notes as far as I can see (https://cmp.onl/tj5G).

I think we could also listen directly to onhashchange to avoid adding permissions, right? https://cmp.onl/tj5y - as long as that isn't restricted in content scripts, but I'm pretty sure it works.

As far as mutation observers, I think if we wait for full window load, we can attach directly to the new note element and listen only for attribute changes - that should be pretty efficient, I believe.

That would take care of new note interaction. We could also listen on the existing note container element for child changes, and attach to each new child to listen to attribute changes, but that is certainly more complicated than listening to URL changes.

So, I'm leaning toward listen to URL change (ideally onhashchange if supported, otherwise, new permissions are acceptable) for existing notes, and mutation observer for new note (https://cmp.onl/tj5G).

I aim to start working on this tomorrow, unless you have any new thoughts. Or if you would prefer to do the PR yourself, I'd be fine with that.

Thanks again!

from chrome-google-keep-full-screen.

1. So you really digged into it ‒ great! While reading I had the idea to use MutationObserver to get notified of changes, but delay the follow up processing. So not not to scan the DOM and run all code right away, but start a timer instead to run the code delayed. This way even multiple fast Mutations wouldn't cause too much trouble.
2. Indeed, creating a new note doesn't change the URL and thus can't be detected like this. So onHasChange wouldn't work either. But a simple onClick listener on that create note element likely works.
3. I don't know whether onHasChange works for detecting these URL changes in Keep.

from chrome-google-keep-full-screen.

I started on this in feature/polling - having trouble listening to the URL change so far (even in console) - it seems like Keep is changing the URL in a way that's hard or impossible to listen to. Forward/Back buttons trigger listeners for popstate and hashchange, but not actually opening/closing notes directly. I think I'll try again to find any custom events Keep may be firing, maybe dig in their code a bit - didn't have any luck with that before, but it would be very nice if they have something we can listen to directly.

On click is a good idea; it wouldn't work with forward/back, and there could be some keyboard interface that wouldn't trigger a click event as well. But, it's worth considering.

from chrome-google-keep-full-screen.

Yeah, onHasChange somehow doesn't work, don't know why. So rather use MutationObserver or onUpdated.

from chrome-google-keep-full-screen.

Solution is ready and working well for the most part, in my testing.

This feels very smooth compared to the tick - so I'm really glad you made the suggestion. I also enjoyed getting a bit more familiar with Mutation Observers.

With the MO method, switching between notes, reminders, archive, etc. all work well, as well as adding new notes. I believe it will be efficient enough, because it is only observing attributes on each note, and the child list on the note group container.

I had 2 concerns:

1. That there might be a memory leak as elements are removed, but, at least according to this post, that appears not to be a concern: https://cmp.onl/tjsL
2. Forward/Back buttons don't work, so I am going to add a listener for that (already know from previous testing that will trigger the URL change events as expected.

I'm also going to prep some small styling fixes and do a beta launch - when that's ready (maybe tomorrow or Monday) would you be willing to help with some testing @MartinLichtblau ? @nickfaughey might you be willing to help test as well? I will tag you both when the beta is ready.

Many thanks!

from chrome-google-keep-full-screen.

Wow, that's an great answer. And yes, of course I wanna test the beta!

from chrome-google-keep-full-screen.

It seems it creates many MutationObserver in a row, ~ 50. See excerpt.

07:18:31.706 script.js:77 new MutationObserver for note
07:18:31.706 script.js:77 new MutationObserver for note
07:18:31.706 script.js:77 new MutationObserver for note
07:18:31.706 script.js:77 new MutationObserver for note
07:18:31.706 script.js:108 checkForOpenNote
07:18:31.707 script.js:108 checkForOpenNote
07:18:31.707 script.js:108 checkForOpenNote
07:18:31.707 script.js:108 checkForOpenNote
07:18:31.708 script.js:108 checkForOpenNote
07:18:31.708 script.js:108 checkForOpenNote
07:18:31.709 

from chrome-google-keep-full-screen.

Good point, we should just create one observer for all notes, since it always uses the same callback (and one to observe the container of notes for when the list changes - so two total).

As for all the extra checkForOpenNote calls, I think that is to be expected with all the attributes Google is changing. I don't see it causing any issues, do you?

Beta is now at 1.2.1.002

from chrome-google-keep-full-screen.

Been looking good on my end, so I'll be launching today and closing this out.

from chrome-google-keep-full-screen.