changes: reduced hints generally
Thats why I found:
Should I start a new ticket to discuss linting? The non consistent syntax makes creating patches without withspace changes very hard.
I don't doubt that some of these changes may well be valid, but I can't just apply them blindly. Can you comment?
I would much prefer removing "id_XXXX":
const pasteboard = $("#pasteboard");
if (window.hasOwnProperty("clipboardData") && window.clipboardData.hasOwnProperty("setData") && typeof window.clipboardData.setData === "function") {
=== "function"
will never match?
Same in:
else if (CLIPBOARD_IMAGES && dtype=="image/png" && dformat==8 && wire_encoding=="bytes" && navigator.clipboard && navigator.clipboard.write) {
hasOwnProperty
change:
clog("file:", f.name, ", type:", f.type, ", size:", f.size, "last modified:", f.hasOwnProperty("lastModifiedDate") ? f.lastModifiedDate.toLocaleDateString() : 'n/a');
if (CLIPBOARD_IMAGES && navigator.clipboard && navigator.clipboard.hasOwnProperty("write")) {
navigator.hasOwnProperty("connection")
etc.. What sort of problems is this meant to prevent?
const id_screen
to const screen
and id_float_menu
to float_menu
//args = args.splice(1);
- why was this commented out?
this._do_connect(false);
packet = ["key-action", me.topwindow, keyname, false, modifiers, keyval, str, keycode, group];
this.protocol = null;
const width = parseInt(this.video.getAttribute("width"));
_tray_geometry_changed
, _tray_set_focus
and _tray_closed
, why add the ctx
argument ?
notification_icon
element?
class_notifications
to notifications
.
const wid = jQuery("#"+String(this.wid));
- wid
has a very specific meaning in xpra, that's the window id, please rename this object to window
Also, please try to keep indentation / semi-colon type of patches separate from the changes that will modify behaviour. We can apply the former very quickly, as it is quick to review, the latter may take more time.
Should I start a new ticket to discuss linting?
I don't mind.
The non consistent syntax makes creating patches without whitespace changes very hard.
We can merge one change that only changes whitespace and nothing else. Then things will be easier?
Replying to Antoine Martin:
I don't doubt that some of these changes may well be valid, but I can't just apply them blindly. Can you comment?
I would much prefer removing "id_XXXX":
const pasteboard = $("#pasteboard");
So remove the id_ part of variables? Or dont remove duplicate jQuery calls?
- skipped this, as it looked wrong:
if (window.hasOwnProperty("clipboardData") && window.clipboardData.hasOwnProperty("setData") && typeof window.clipboardData.setData === "function") {
=== "function"
will never match?
typeof window.clipboardData.setData === "function"
will match
Same in:
else if (CLIPBOARD_IMAGES && dtype=="image/png" && dformat==8 && wire_encoding=="bytes" && navigator.clipboard && navigator.clipboard.write) {
- I didn't really see the point of most of these
hasOwnProperty
change:
clog("file:", f.name, ", type:", f.type, ", size:", f.size, "last modified:", f.hasOwnProperty("lastModifiedDate") ? f.lastModifiedDate.toLocaleDateString() : 'n/a');if (CLIPBOARD_IMAGES && navigator.clipboard && navigator.clipboard.hasOwnProperty("write")) {navigator.hasOwnProperty("connection")etc.. What sort of problems is this meant to prevent?
With clipboardData.setData
the attribute is accessed, clipboardData.hasOwnProperty("setData")
checks if there is such an attribute (what was meant with clipboardData.setData
). On expecting a function, typeof window.clipboardData.setData === "function"
needs to be used.
It results in no access to undefined objects (and also no warnings).
- rename
const id_screen
toconst screen
andid_float_menu
tofloat_menu
//args = args.splice(1);
- why was this commented out?
It is commented out to point out that it is not used, can be deleted.
- these look like bug fixes:
this._do_connect(false);
packet = ["key-action", me.topwindow, keyname, false, modifiers, keyval, str, keycode, group];
this.protocol = null;
const width = parseInt(this.video.getAttribute("width"));
Yes
- in
_tray_geometry_changed
,_tray_set_focus
and_tray_closed
, why add thectx
argument ?
ctx is used in the line below like ctx.debug("tray", "tray geometry changed (ignored)");
- why change the type of the
notification_icon
element?
This is an error. My intention was to only delete the closing img tag (</img>) as this does not exists.
- rename
class_notifications
tonotifications
.
I wanted to get rid of the duplicate jQuery call, so I introduced a variable class_notifications.
const wid = jQuery("#"+String(this.wid));
-wid
has a very specific meaning in xpra, that's the window id, please rename this object towindow
window is in used by JS, what about window_element?
Also, please try to keep indentation / semi-colon type of patches separate from the changes that will modify behaviour. We can apply the former very quickly, as it is quick to review, the latter may take more time.
Like this? These are not incremental, is this even possible? Do you create a separate branch on merging?
Like this?
Perfect, thank you.
Updates:
ctx
to the method, or use this
, the window calls the geometry callback using: this.geometry_cb(this);
.
lineWidth
is a number
clipboardData
-> window.clipboardData
Not done yet:
hasOwnProperty
- avoiding warnings is nice, the syntax is not
Only problem is that something broke and makes the html5 client unusable:
Uncaught ReferenceError: Buffer is not defined at Object.<anonymous> (forge.js:25985) at __webpack_require__ (forge.js:30) at Object.<anonymous> (forge.js:25412) at __webpack_require__ (forge.js:30) at Object.<anonymous> (forge.js:25391) at __webpack_require__ (forge.js:30) at forge.js:73 at forge.js:76 at webpackUniversalModuleDefinition (forge.js:9) at forge.js:10
Uncaught ReferenceError: Buffer is not defined
Fixed in r25953.
Updates:
hasOwnProperty
for something which is never defined)
hasOwnProperty
: I'm going to hold off on this one:
navigator.clipboard.write ƒ () { [native code] } navigator.clipboard.write === "function" false navigator.clipboard.write == "function" false
r25965 fixes uglifyjs
using regexp substitutions, so we lose the "const" and "let" changes during minification... This will have to do until we switch to terser (#2709).
@brief: can I close this or do you want to add more?
This should be the last one.
Replying to Antoine Martin:
- still about
hasOwnProperty
: I'm going to hold off on this one:navigator.clipboard.write ƒ () { [native code] } navigator.clipboard.write === "function" false navigator.clipboard.write == "function" false
I only made one patch now since this looks like a line mismatch while merging.
Thanks!
From the "final" patch:
As per comment:8, the check for === "function"
does not work under chrome, so I have ignored that.
Also, I am not merging the const Buffer
and const LZ4
because of the problems this causes, see comment:7 (though the problem may not show since we replace const
by var
before using the minifier - nonetheless, AFAICT this is still a problem)
Closing, let's continue this in a new ticket for 4.1
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/2705