Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TF window opening outside the display #189

Closed
Procyon-b opened this issue Sep 27, 2019 · 15 comments · Fixed by #192
Closed

TF window opening outside the display #189

Procyon-b opened this issue Sep 27, 2019 · 15 comments · Fixed by #192
Assignees
Labels
bug confirmed Bugs that have been reproduced by one of the developers infrastructure Issues other than direct user-facing functions window-management Position and size of the TF window or of Chrome windows

Comments

@Procyon-b
Copy link
Contributor

Procyon-b commented Sep 27, 2019

I recall reading another issue mentioning this bug, but I can't find it. (Edit from cxw #18 and #141 are closest; also related to #111)

I'm running chrome with TF on a small screen (1280x800). It works well for everything but all windows are maximized (not TF obviously). It's probably the reason for this bug to happen: I have used TF on another computer with a much bigger display, chrome not maximized, and never TF out of sight.

chrome.storage.local.get( "tabfern-window-location", function(items){
  console.info(items);
  console.info( JSON.stringify(items) );
});

Gives always the same value in case of error:
{"tabfern-window-location":{"height":200,"left":-32000,"top":-32000,"width":160}}

To work around this problem, I have devised these scriplets:

chrome.storage.local.get( function(items){
  // create a backup copy
  chrome.storage.local.set(
    {"tabfern-window-location-bak": items["tabfern-window-location"] }
    );
});

// this one to restore the saved values:
chrome.storage.local.get( function(items){
  chrome.storage.local.set(
    {"tabfern-window-location": items["tabfern-window-location-bak"] }
    );
});

I have used scriptlet 1 once, and since then I use scriplet 2 to "reset" TF window:
focus TF window (even if not visble), F12 to open console, run script 2, close devtools, F5 (reload TF window)

@cxw42
Copy link
Owner

cxw42 commented Sep 28, 2019

Yes, there is another issue open - I'll find it. You can also double-click the TF icon in any Chrome window to summon the TF window to that Chrome window. Position backup is an interesting idea - we could theoretically support any number of saved window positions.

@Procyon-b
Copy link
Contributor Author

You can also double-click the TF icon in any Chrome window to summon the TF window

But the size and position wouldn't be identical to the previous saved infos (IIRC).

we could theoretically support any number of saved window positions.

exactly what I though

@cxw42
Copy link
Owner

cxw42 commented Sep 28, 2019

Summon changes position but not size, as I recall. Anyway, PRs welcome! :)

@Procyon-b
Copy link
Contributor Author

Anyway, PRs welcome! :)

;)

I'll look into it, and see what I can do.

@Procyon-b
Copy link
Contributor Author

Procyon-b commented Sep 28, 2019

Summon changes position but not size, as I recall.

I've just got the error right now, and despite the stored value of {"height":200,"width":160} , when I double click, the size is not that at all. I think it's window 7 that is shrinking the window when it opens it outside the viewport.
In fact, it is the values used. TF is screwing the position and the dimensions.

My backed-up values are {"tabfern-window-location":{"height":750,"left":3,"top":2,"width":281}}

@cxw42
Copy link
Owner

cxw42 commented Sep 28, 2019

Related to #111 and #141, but neither is exactly the same as this one :) . I'll tag it.

Summon is:

function moveTabFernViewToWindow(reference_cwin)
{
function clip(x, lo, hi) { if(hi<lo) hi=lo; return Math.min(hi, Math.max(lo, x)); }
if(!isLastError()) {
if(!me.viewWindowID) return;
chrome.windows.get(me.viewWindowID, function(view_cwin) {
// TODO handle Chrome error
let updates = {left: reference_cwin.left+16,
top: reference_cwin.top+16,
state: 'normal', // not minimized or maximized
};
// Don't let it be too large or too small
updates.width = clip(view_cwin.width, 200, reference_cwin.width-32);
updates.height = clip(view_cwin.height, 100, reference_cwin.height-32);
chrome.windows.update(me.viewWindowID, updates
// TODO handle Chrome error
);
});
}
} //moveTabFernViewToWindow()

So, yes, summon does currently clip size based on the Chrome window to which you summon it.

So the backed-up position is (3,2) --- where did the window appear? If you do a chrome.windows.get when the TF window is offscreen, what size/position do you get for the TF window?

@cxw42 cxw42 added window-management Position and size of the TF window or of Chrome windows bug labels Sep 28, 2019
@Procyon-b
Copy link
Contributor Author

Procyon-b commented Sep 28, 2019

I'll check chrome.windows.get as soon as I get the error again.

Here is the procedure I always follow when closing chrome:

I close all windows one at a time, always finishing with my first/main window (if it has pined tabs).
Then the only window opened is TF; Sometimes minimized at the time (but I "restore" it); and I close it.
I close TF last to prevent "Recovered tabs" from appearing.

The solution I had in mind for some time now, is to prevent TF storing new values if "left" and/or "top" are way off in the negative value.

@Procyon-b
Copy link
Contributor Author

Procyon-b commented Sep 28, 2019

alwaysOnTop: false
focused: false
height: 200
id: 1084414374
incognito: false
left: -32000
state: "normal"
top: -32000
type: "popup"
width: 160

Edit:
Isn't 160x200 the default size when TF opens its window?

One thing I'm wondering now. Is {"tabfern-window-location":{"height":200, "left":-32000, "top":-32000, "width":160}} stored at closing time, or when TF opens its window (with faulty values, different from these).

Edit:
{"tabfern-window-location":{"height":27,"left":-32000,"top":-32000,"width":160}} is the value actually stored (from the background page console, before opening TF window).

@cxw42
Copy link
Owner

cxw42 commented Sep 30, 2019

That -32000 is definitely not right :) .

I had one thought - in two places (lines 2962 and 2976), we getWindowSize(window):

TabFern/app/win/main_tl.js

Lines 2953 to 2981 in 27d8b2d

/// When the user resizes the tabfern popup, save the size for next time.
function eventOnResize(evt)
{
// Clear any previous timer we may have had running
if(resize_save_timer_id) {
window.clearTimeout(resize_save_timer_id);
resize_save_timer_id = undefined;
}
let size_data = getWindowSize(window);
// Save the size, but only after two seconds go by. This is to avoid
// saving until the user is actually done resizing.
resize_save_timer_id = window.setTimeout(
()=>{saveViewSize(size_data);}, 2000);
} //eventOnResize
// On a timer, save the window size if it has changed. Inspired by, but not
// copied from, https://stackoverflow.com/q/4319487/2877364 by
// https://stackoverflow.com/users/144833/oscar-godson
function timedResizeDetector()
{
let size_data = getWindowSize(window);
if(!ObjectCompare(size_data, last_saved_size)) {
saveViewSize(size_data);
}
setTimeout(timedResizeDetector, K.RESIZE_DETECTOR_INTERVAL_MS);
} //timedResizeDetector

In that code, window is static/win/main.html. What we actually care about is the size of the container --- static/win/container.html. I wonder if we are sometimes getting a position/size of main with respect to container instead of with respect to the screen?

I'm not sure how to test that other than logging, but I am interested to know if you have any thoughts.

@cxw42
Copy link
Owner

cxw42 commented Sep 30, 2019

The solution I had in mind for some time now, is to prevent TF storing new values if "left" and/or "top" are way off in the negative value.

We can probably do that for -32000 :) . But eventually monitors will be that large, and the code will break! I am hoping we can figure out the root cause of the -32000.

If worst comes to worst, we can make a profiling version of TF and run a timer to check every frame whether the numbers have gone wonky. That would get us closer (at a rather severe cost).

@cxw42 cxw42 assigned cxw42 and Procyon-b and unassigned cxw42 Sep 30, 2019
@cxw42 cxw42 added the infrastructure Issues other than direct user-facing functions label Sep 30, 2019
@Procyon-b
Copy link
Contributor Author

Procyon-b commented Sep 30, 2019

In that code, window is static/win/main.html. What we actually care about is the size of the container --- static/win/container.html. I wonder if we are sometimes getting a position/size of main with respect to container instead of with respect to the screen?

I've checked.
When TF windows is minimized,

chrome.storage.local.get( "tabfern-window-location", function(items){
  console.info(items);
  console.info( JSON.stringify(items) );
});

consistently gives {"tabfern-window-location":{"height":27,"left":-32000,"top":-32000,"width":160}}, and once restored, {"tabfern-window-location":{"height":750,"left":3,"top":2,"width":281}}

Edit:
Maybe its OS specific, or browser version specific.
I've examined the window. object, and can't find reference to the normal value, or to the restored/minimized status.

Edit:
Maybe checking outerHeight for a very low value (=27 when minimized) to detect ?

@cxw42
Copy link
Owner

cxw42 commented Sep 30, 2019

Ah! Yes, indeed --- that's a leftover from Win95, as I recall (nope - WinNT - Old New Thing). I see also this SO question about the same. Well, whaddya know!

Confirmed!

With the developer console set to tree-iframe (main.html), and the TF window visible:

> chrome.windows.get(my_winid, (data)=>{console.log(data);});
alwaysOnTop: false focused: false height: 535 id: 25 incognito: false left: 1527 
    state: "normal" top: 500 type: "popup" width: 383 __proto__: Object

After minimizing TF:

> chrome.windows.get(my_winid, (data)=>{console.log(data);});
alwaysOnTop: false focused: false height: 535 id: 25 incognito: false left: 1527 
    state: "minimized" top: 500 type: "popup" width: 383 __proto__: Object

Now, shortly after that, I got ---

main.js:37129 Saved size

because the resize-detection timer expired and timedResizeDetector() ran. I checked the resulting saved state and saw what you saw:

> chrome.storage.local.get(K.LOCN_KEY,(data)=>{console.log(data);})
tabfern-window-location: {height: 24, left: -32000, top: -32000, width: 160} __proto__: Object

I then restored the TF window and:

main.js:37129 Saved size
> chrome.storage.local.get(K.LOCN_KEY,(data)=>{console.log(data);})
tabfern-window-location: {height: 535, left: 1527, top: 500, width: 383} __proto__: Object

Fix

So the fix should be as simple as:

  • Only use chrome.windows.get data for TF size/position data. Don't use window.<foo>.
  • Whenever we are about to save, call chrome.windows.get(my_winid, ...). Don't change the saved position if the resulting state is "minimized".

As far as I know, only app/win/main_tl.js will need to change.

Would you be willing to send in a PR? Don't do so until Oct. 1 ;) . Thanks!

Additional confirmation

I minimized TF, waited until it saved size, closed it without restoring it, then hit the toolbar button. It appeared in the UL corner of the screen for a moment, then disappeared. In the console, the saved-position data indeed had the -32000.

Thanks for your help figuring this out!

@cxw42 cxw42 added the confirmed Bugs that have been reproduced by one of the developers label Sep 30, 2019
@cxw42
Copy link
Owner

cxw42 commented Oct 1, 2019

Please see updated contribution guidelines. Thanks!

@Procyon-b
Copy link
Contributor Author

I have tried a fix on a clone of the current version 0.2.0.1340. It works.
The fix relies only on verifying the value .state == 'normal' (from chrome.windows.get(my_winid, ... ).
eventOnResize(evt) and timedResizeDetector() still get their values from getWindowSize(window).

Question:
Can we retire getWindowSize() entirely?
basicInit() would also need to be modified.
chrome.windows.get() gives us the necessary values. The only difference is that this function is async. Not a problem with the 2 timer/event based functions above, but I haven't looked at basicInit() yet, and how it is used.

Side note
I'll look at the guidelines on how to install the current code.
I'm on a win7/32. Is it a problem?
I know that github desktop is only 64bits. I suppose that command-line git is compatible, and other utils too.

@cxw42
Copy link
Owner

cxw42 commented Oct 1, 2019

Win7 32 should be fine - windows git and Cygwin both still support x86, as far as I know.

I think using only chrome.windows.get would be fine for this. The async has only been an issue for me in the past in drag-and-drop code.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed Bugs that have been reproduced by one of the developers infrastructure Issues other than direct user-facing functions window-management Position and size of the TF window or of Chrome windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants