Preferred window size and position offsets #222
Conversation
There was a problem hiding this comment.
@csabag Thanks for your good work on this! I have some specific questions and comments related to robustness and style. Please let me know your thoughts. Much appreciated!
Note: if we need to modify the settings library, I am happy to help.
| // When a window is created | ||
| chrome.windows.onCreated.addListener(function(window){ | ||
| // Leave TabFern window alone... | ||
| if (window.id === me.viewWindowID) return; |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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]+)$/) |
There was a problem hiding this comment.
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.
| leftOffset = parseInt(fixedOffsetValues[2]); | ||
| chrome.windows.update(window.id, {top:topOffset, left:leftOffset}); | ||
| } | ||
| let relativeOffsetFormat = RegExp(/^\+([0-9]+):\+([0-9]+)$/) |
There was a problem hiding this comment.
Since we are allowing positive relative offsets, I would like to allow negative relative offsets as well!
| _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]+$/); |
There was a problem hiding this comment.
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.
| "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`) |
There was a problem hiding this comment.
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.
| "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> |
There was a problem hiding this comment.
Does this show up OK? I can't remember if it needs to be "html" instead of "text".
| }, | ||
| ]; | ||
|
|
||
| if(S.getBool(S.FIXED_WINDOW_SIZE)) setting_definitions.push( |
There was a problem hiding this comment.
Style: please add { } around the settings_definitions.push for consistency with in the codebase.
Adding preferred window size and position offsets settings.
Fixes #182.