window size/pos update only with correct values

[Procyon-b]

Fixes #189.

eventOnResize() and timedResizeDetector() use chrome.windows.get()
then getWindowSize(window); replaced by getWindowSizeFromWindowRecord(cwin);

Only basicInit() (from main() ) still uses getWindowSize().
Since all things must be done in order, and chrome.windows.get() is async, if we decide to also use a cwin there, is there a danger of newWinSize being read during main() before it is actually set?

[cxw42]

Thank you, and good question! I'll take a look. There are numerous places in the codebase that trigger ASQ sequences off Chrome callbacks, so I don't think ordering will be a problem if we need it.

[cxw42]

@Procyon-b I updated the logic in basicInit and changed it to permit saving size of maximized windows as well as normal windows. Would you please test and see what you think? Thanks!

[Procyon-b]

Just a quick remark before leaving keyboard (and before testing).
I excluded maximized because, like with minimized, it will erase normal size/pos. If a user closes chrome with TF maximized (and size saved), the next time chrome is started, TF would be opened with maximized size, with normal infos lost.

[Procyon-b]

I've just installed git, node.js & all, downloaded the latest code (easy, no problem), and tried.
It's what I expected. storing size/pos of the maximized window is wrong. The user can maximize the window anyway, without having the values stored.

Picture this. I rarely maximize TF, but sometimes do it to view the dialog ("edit text"), or to view a particularly long tab's title. If somehow chrome gets closed, I'm in the same situation as I'm currently regarding the minimized values.

[cxw42]

Fair point. I was thinking of the flip side - if a person on a multi-monitor setup leaves TF maximized on one monitor, we would be throwing that away. I think we can:

• Try just normal for now, and see what happens; or
• Add an option controlling whether or not maximized size will be saved.

What do you think?

[Procyon-b]

I understand why maximized could be important too.

With the chrome window we get the .state value. I can see a way to use it.

Store the "state" parameter as well. Update it when value is "normal" or "maximized", ignore when it is "minimized".
Store size and pos only when state is "normal".

Then, when TF opens its window, add a step:

• open with default size/pos
• resize/move using stored values
• maximize if stored "state" is "maximized".
  Is it possible, and on the correct monitor?

This way the user can go in and out of maximized mode while retaining the "normal" window size.

Edit:
Oh, and we forgot one state: "fullscreen".
It should probably be treated the same way if TF can go in fs all by itself.

[cxw42]

@Procyon-b Sorry for the delay in responding!

• I agree with your proposal in window size/pos update only with correct values #192 (comment) .
• I just tried F11, and TF can go fullscreen, although I had never tried it before :D .

I look forward to a PR update at your convenience!

[Procyon-b]

I had a big cold, and tonight I'm finally feeling ok. ;)
In the following days I'll try to figure how all this can be coded.

[cxw42]

Sorry you've been under the weather — glad to hear you're back on your feet! If you want to talk live sometime, we can discuss on Gitter if you like.

[Procyon-b]

I'm not forgetting but I had other things to do before this. ;)

[Procyon-b]

Question:
I have a working version, but since this PR has been created, the source code in your repository has been modified. How am I suppose to update the code without losing the commits added in your version?
Is it better to close this PR and create a new one (after syncing my fork)?

[cxw42]

@Procyon-b Thanks! No need to open a new PR. When you push your latest commit(s), GitHub will tell you if there are any conflicts. If not, I will be able to merge cleanly. If there are conflicts, either of us can add a commit to fix them. I am happy to do that if necessary.

From a quick look at the commits since your branch split off, I don't anticipate any significant conflicts.

At work, we have to merge/rebase/fix commits all the time --- this situation is not a problem :) .

(Note: take a look at the network graph --- you'll see lots of places where parallel commits were merged together.)

[Procyon-b]

PR updated.
One file is updated, main_tl.js, and the modifications are based on your current code. The other, (unmodified) files of my fork are of the version of early october.

Here is how it works:

• chrome.windows.get() is called to get the current size+pos of the window
• the values are used only if the window is in normal state. If not, a copy is used, or the last saved values set is used
• the current set of values is stored in a temp var (necessary since last_saved_size will only be updated when the timer is executed)
• window state is added to the object
• ObjectCompare() is added in the timer and the timer delay is reduced to 200. (using console.log we could see that this reduces significantly the number of savings, while allowing the last window status to be saved before the user closes the window. 2s is long)
• the interval timer only saves when the window state is normal (it's only needed to catch displacements of the window). storing of window state is also updated there.
• one line is added to moveWinToLastPositionIfAny_catch() to restore the window state (it must be a separate function call)

[cxw42]

No conflicts :D . Thanks - I'll take a look!

[cxw42]

@Procyon-b I apologize for the long delay in looking at this! I agree with your approach of only saving normal window size/position for now. Would you please test the updates I have pushed and see if they work for you?

The changes relevant to this PR are in 55524bd. I have merged in some unrelated bugfixes I made this morning, which is why there are five other commits before 55524bd in the PR now.

Edit I have added a few explanatory comments in 55524bd. I hope they help! If not, please disregard them.

[Procyon-b]

I've completely forgotten about this. Sorry.
I'll try to grab some time this week-end.

[cxw42]

No problem! That's what happens when neither of us is paid to work on this :D :D 😆

[Procyon-b]

I'm back. My mind is lighter. I had an extension (Tiny Proxy) that was stuck in eternal "pending review"/"Rejected" state. :)

I've tested this code, and it still works as expected.

cur_size : I used it to keep track of the very last size of the window in case the window was maximized/minimized before the timer had the chance to run (and save the values). In that specific case, the last values are lost. But with a low timeout value (300), we can do without it.

[Procyon-b]

I'm still using version 0.2.0.

I'm using chrome with windows maximized (small screen). When I click in TF (no maximized) to open a saved set of tabs, it sometimes uses weird size values for the opened window.

• OK: the window opens not maximized with the correct current size
• not OK: the window opens not maximized with the size as large as possible (window border on the edge of the screen)
• not OK: at least once, I have seen the new window duplicating the size of TF window

I haven't look at the code, but could this be related to the problem we had to solve here? The actual size of the window is lost when the page is not normal.

[cxw42]

Good point - that is very possible! The code was originally trying to simulate the Ctrl+N size/pos because of apparent Chrome limitations. First, I should check whether those limitations still exist. If so, code like this may be very helpful.

Congratulations on finally getting your extension approved!

I currently can't physically access my desk :) but will get this merged as soon as I can!

[Procyon-b]

Congratulations on finally getting your extension approved!

It was approved, but updates where denied.

In early december, the last approved update took 7-8 days to be accepted. The previous update, not too long before, was immediate. I've submitted the next update the day of the approval, but it took more than 10 days to be rejected. Fortunately the extension was not blocked.

What is difficult is getting information from google. The email warning states that you can ask questions and the "developper support team" will follow up. The fact is that they don't. Except for replying with a similar error message.
The update was rejected on the ground that one of the permissions was either too broad, or wasn't used, or was the wrong one to implement the feature. It's too vague to know exactly what to do. But it could only be "activeTab" since the others have no alternatives: "proxy", "contextMenus".

I even followed someone's advice by mailing another dev team. It was mid-december and the (nice) reply only came mid-january. But they couldn't help with that specific problem.

I let it rest for a month (or more), and finally wrote a new email with very specific questions. The reply came with the usual error message, but with a little hint on which permission was at fault. But not why.

So, I replace "activeTab" with "<all_urls>" and after 5 days of review, the update was accepted.

In fact I don't think that they know exactly what they have to do. When submitting the new update, the dev dashboard asked me if I didn't want to replace "<all_urls>" by "activeTab". Which is contrary to what I was asked/hinted to do by the support team.

What could have made a difference is that I had to fill the "privacy" form of that extension in the new dashboard interface. There you can comment/explain each permission. Comments that are passed to the reviewer.

[cxw42]

Wow --- crazy! Thanks for the tip --- I will make sure to fill in the privacy section if I ever have to change permissions.

[Procyon-b]

Note that when the update was rejected, the manifest hasn't been modified. Only some bugs fixed in options.js and background.js.
So in a matter of hours - between the approval of v0.6.0 and the submission of v0.6.1 - plus the days of reviewing 0.6.1, "activeTab" was deemed problematic.

[Procyon-b]

If you want to know this is the online help.
And here is why I needed "activeTab": to get the hostname of the tab

[cxw42]

I have merged this into the current rc branch for testing!

[Procyon-b]

[off topic]
re: the new dashboard.

In the old dashboard, they have replaced the link to the new dashboard with an "opt-in now" link. Fortunately there is an hidden "opt-out" link in the new interface.
The new interface lacks the "user feedback" interface, and probably other things too. So, from my point of view, I still need both versions.
My solution it to keep the dashboard in the old version mode, and open the new one with a direct link: https://chrome.google.com/webstore/devconsole/

[cxw42]

Thanks for the tip! I will do likewise.

[cxw42]

TF rc branch has been doing OK for me this week. I should be able to release in the next few days :) .

[cxw42]

0.2.1 is pending review - fingers crossed! I'm not going to tag or merge to master until it's approved, in case I have to make changes during the review process.