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.

Sign up for GitHub

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

Preferred window size and position offsets #222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Preferred window size and position offsets #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions app/bg/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,37 @@ let onCommandListener = function(cmd) {
} //onCommandListener()
chrome.commands.onCommand.addListener(onCommandListener);

// When a window is created
chrome.windows.onCreated.addListener(function(window){
// Leave TabFern window alone...
if (window.id === me.viewWindowID) return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if viewWindowID will always exist when this function is called. E.g., I don't know if Chrome will give us window-creation messages for the first window that opens before the TF popup opens. Would you please add a check that viewWindowID is defined before using it?


if (S.getBool(S.FIXED_WINDOW_SIZE) && window.type === 'normal') {
let windowSize = S.getString(S.S_WINDOW_SIZE);
[windowWidth, windowHeight] = windowSize.split('x').map((value) => parseInt(value));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a string of WxH instead of separate width and height fields? (And the same throughout.)

E.g., what if the user types 800 x 600 (with spaces around the x) or 800 (forgetting the x H)? I personally would prefer two separate numeric fields.

However, I do see an advantage of a single field. With a single field, it's either set or it's not. Perhaps we could change settings.js to improve the validation.

What are your thoughts?

chrome.windows.update(window.id, {width: windowWidth, height: windowHeight});
}
if (S.getBool(S.PREFERRED_WINDOW_POSITION_OFFSET) && window.type === 'normal') {
let fixedOffsetFormat = RegExp(/^([0-9]+):([0-9]+)$/)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please change let to const here and elsewhere that we are creating regexes or other constants.

Also, same question about one field vs. two.

let fixedOffsetValues = S.getString(S.S_WINDOW_POSITION_OFFSET_VALUES).match(fixedOffsetFormat);
if(fixedOffsetValues) {
topOffset = parseInt(fixedOffsetValues[1]);
leftOffset = parseInt(fixedOffsetValues[2]);
chrome.windows.update(window.id, {top:topOffset, left:leftOffset});
}
let relativeOffsetFormat = RegExp(/^\+([0-9]+):\+([0-9]+)$/)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are allowing positive relative offsets, I would like to allow negative relative offsets as well!

let relativeOffsetValues = S.getString(S.S_WINDOW_POSITION_OFFSET_VALUES).match(relativeOffsetFormat);
if(relativeOffsetValues) {
// According to Window features documentation default new window offset is 22px
// https://developer.mozilla.org/en-US/docs/Web/API/Window/open#Window_features
// If user has a preferred relative offset preference we need to discount the default offset
topOffset = window.top + parseInt(relativeOffsetValues[1])-22;
leftOffset = window.left + parseInt(relativeOffsetValues[2])-22;
chrome.windows.update(window.id, {top:topOffset, left:leftOffset});
}
}
});

// When a window is closed
chrome.windows.onRemoved.addListener(function(windowId) {
// If the window getting closed is the popup we created
Expand Down
22 changes: 22 additions & 0 deletions app/common/setting-definitions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,14 @@ _NAM.CFG_TITLE_IN_TOOLTIP = 'tooltip-has-title';
_DEF[_NAM.CFG_TITLE_IN_TOOLTIP] = false;
_VAL[_NAM.CFG_TITLE_IN_TOOLTIP] = _vbool;

_NAM.CFG_FIXED_WINDOW_SIZE = 'window-size-is-fixed';
_DEF[_NAM.CFG_FIXED_WINDOW_SIZE] = false;
_VAL[_NAM.CFG_FIXED_WINDOW_SIZE] = _vbool;

_NAM.CFG_PREFERRED_WINDOW_POSITION_OFFSET = 'window-position-offset-is-fixed';
_DEF[_NAM.CFG_PREFERRED_WINDOW_POSITION_OFFSET] = false;
_VAL[_NAM.CFG_PREFERRED_WINDOW_POSITION_OFFSET] = _vbool;

/// Not actually a setting, but an indicator that we loaded settings OK.
/// Used by src/settings/main.js.
_NAM.SETTINGS_LOADED_OK = '__settings_loaded_OK';
Expand Down Expand Up @@ -168,6 +176,20 @@ _VAL[_NAM.CFGS_FAVICON_SOURCE] = (v)=>{
return (( v === FAVICON_SITE || v === FAVICON_CHROME || v === FAVICON_DDG ) ? v : undefined);
};

_NAM.CFGS_WINDOW_SIZE = 'window-size';
_DEF[_NAM.CFGS_WINDOW_SIZE] = '800x600';
_VAL[_NAM.CFGS_WINDOW_SIZE] = (v)=>{
let regex = RegExp(/^[0-9]+[xX][0-9]+$/);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validator does not match the parsing above (windowSize.split('x')). If we stay with the single-field approach, I would like to move the regex or other logic into an export from S so that we only have to make changes in one place.

return (( v.match(regex) ) ? v : undefined);
};

_NAM.CFGS_WINDOW_POSITION_OFFSET_VALUES = 'window-position-offset-values';
_DEF[_NAM.CFGS_WINDOW_POSITION_OFFSET_VALUES] = '+36:+36';
_VAL[_NAM.CFGS_WINDOW_POSITION_OFFSET_VALUES] = (v)=>{
let regex = RegExp(/^\+?[0-9]+:\+?[0-9]+$/);
return (( v.match(regex) ) ? v : undefined);
};

// }}}2

/// The default values for the configuration settings.
Expand Down
74 changes: 72 additions & 2 deletions app/settings/manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,77 @@ vote at ${issue(125,true)}.
"type": "checkbox",
"label": future_i18n('Sort open windows to the top, scroll to the top of the list'),
},
{
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"name": S.FIXED_WINDOW_SIZE,
"type": "checkbox",
"label": future_i18n('Open a window, always open it with a preferred fixed size'),
},
{ // some extra descriptive text for S.FIXED_WINDOW_SIZE
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"type": "description",
"text": future_i18n(`If you change this value please refresh Settings
page (this page) to reveal preferred window size parameter`)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give me a bit more detail about this? We can update the settings page at load time (app/settings/settings.js. main()) if we need to populate a field or customize something.

},
];

if(S.getBool(S.FIXED_WINDOW_SIZE)) setting_definitions.push(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: please add { } around the settings_definitions.push for consistency with in the codebase.

{
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"name": S.S_WINDOW_SIZE,
"type": "text",
"label": future_i18n('Window size'),
},
{ // some extra descriptive text for S.FIXED_WINDOW_SIZE
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"type": "description",
"text": future_i18n(`The format of the window size parameter is <em>[width]x[height]</em>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this show up OK? I can't remember if it needs to be "html" instead of "text".

e.g. <strong>800x600</strong>`)
},
);

setting_definitions.push(
{
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"name": S.PREFERRED_WINDOW_POSITION_OFFSET,
"type": "checkbox",
"label": future_i18n('Open a window, use preferred window position offset'),
},
{ // some extra descriptive text for S.PREFERRED_WINDOW_POSITION_OFFSET
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"type": "description",
"text": future_i18n(`Refresh Settings page (this page) when changed
to reveal preferred window position offset parameter`)
},
)

if(S.getBool(S.PREFERRED_WINDOW_POSITION_OFFSET)) setting_definitions.push(
{
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"name": S.S_WINDOW_POSITION_OFFSET_VALUES,
"type": "text",
"label": future_i18n('Window position offset'),
},
{ // some extra descriptive text for S.FIXED_WINDOW_SIZE
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
"type": "description",
"text": future_i18n(
`If format is <em>[top]:[left]</em> e.g. <strong>16:16</strong> the offset is relative to <strong>screen</strong><br/>
If format is <em>+[top]:+[left]</em> e.g. <strong>+36:+36</strong> the offset is relative to <strong>last focused window</strong><br/>
<strong>+0:+0</strong> opens new window exactly <strong>on top of previously focused window</strong><br>
<strong>0:0</strong> opens new window exactly <strong>positioned top left corner of screen</strong><br>`)
},
);

setting_definitions.push(
{
"tab": future_i18n("Behaviour"),
"group": future_i18n("When I..."),
Expand All @@ -159,8 +230,7 @@ vote at ${issue(125,true)}.
TabFern window, even if you didn't check the "Sort
open windows" box above.`
},

]; //setting_definitions
);

if(S.ISSUE35) setting_definitions.push(
{
Expand Down
111 changes: 32 additions & 79 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.