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

fix: don't try to deepMerge read-only properties #14206

Open
wants to merge 2 commits into
base: 2.5
Choose a base branch
from

Conversation

Swiftb0y
Copy link
Member

Fixes #14197
Please confirm @git-developer

@git-developer
Copy link
Contributor

Does not work, unfortunately.

I suppose that instead of or in addition to the source property, the target property needs to be taken into account:

            // ensure the property is not read-only, if so skip it.
            if (!Object.getOwnPropertyDescriptor(source, key).writable) {
                return;
            }
            const targetProperty = Object.getOwnPropertyDescriptor(target, key);
            const targetReadonly = targetProperty !== undefined && !targetProperty.writable
            if (targetReadonly) {
                return;
            }
            if (
                Array.isArray(target[key]) && Array.isArray(source[key]) ||
              isSimpleObject(target[key]) && isSimpleObject(source[key])
            ) {
                deepMerge(target[key], source[key]);
            } else if (source[key] !== undefined && source[key] !== null) {
                print("Key: " + key);
                print("Source: "); printObject(Object.getOwnPropertyDescriptor(source, key));
                print("Target: "); printObject(Object.getOwnPropertyDescriptor(target, key))
                Object.assign(target, {[key]: source[key]});
            }

Result:

 js:debug [Controller] Key: id
 js:debug [Controller] Source:
 js:debug [Controller] {
  "value": "{3d512b38-c006-41cf-989f-dcd0759d07ab}",
  "writable": true,
  "enumerable": true,
  "configurable": true
}
 js:debug [Controller] Target:
 js:debug [Controller] {
  "value": "{31285c70-bd41-4134-9a8c-fc4d66fa6b05}",
  "writable": true,
  "enumerable": true,
  "configurable": true
}
 controller.midi1x1midi1:warning [Controller] ControllerScriptHandlerBase: "Uncaught exception: file:///home/christian/.mixxx/controllers/common-controller-scripts.js:200:
TypeError: Cannot assign to read-only property \"id\"

Backtrace: @file:///home/christian/.mixxx/controllers/common-controller-scripts.js:200
deepMerge@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:180
@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:194
deepMerge@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:180
deepMerge@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:176
@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:194
deepMerge@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:180
@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:194
deepMerge@file:///home/christian/.mixxx/controllers/common-controller-scripts.js:180
applyLayer@file:///usr/share/mixxx/controllers/midi-components-0.0.js:663
shift@file:///usr/share/mixxx/controllers/Behringer-Extension-scripts.js:1114
@file:///usr/share/mixxx/controllers/midi-components-0.0.js:614
applyOperationTo@file:///usr/share/mixxx/controllers/midi-components-0.0.js:536
forEachComponent@file:///usr/share/mixxx/controllers/midi-components-0.0.js:548
shift@file:///usr/share/mixxx/controllers/midi-components-0.0.js:593
inSetValue@file:///usr/share/mixxx/controllers/Behringer-Extension-scripts.js:154
input@file:///usr/share/mixxx/controllers/midi-components-0.0.js:184
input@file:///usr/share/mixxx/controllers/Behringer-Extension-scripts.js:1142
input@file:///usr/share/mixxx/controllers/Behringer-Extension-scripts.js:1309
@:1"

So the property is writable and at the same time read-only? 😵
Maybe the problem is related to the way how the connection object is created?

@Swiftb0y
Copy link
Member Author

I suppose that instead of or in addition to the source property, the target property needs to be taken into account:

Yes, of course...

So the property is writable and at the same time read-only? 😵
Maybe the problem is related to the way how the connection object is created?

Note quite sure, though its probably a missing Qt feature...

Q_PROPERTY(QString id READ readId)
Q_PROPERTY(bool isConnected READ readIsConnected)

In that case, a try catch is probably better.

@Swiftb0y
Copy link
Member Author

try the newest commit please.

@git-developer
Copy link
Contributor

The try-catch variant heals the crash, but the behavior (shifting layers in my case) is not working as expected. Maybe deepMerge() has more differences to lodash's _.merge() than expected. It takes more time to investigate.

@Swiftb0y
Copy link
Member Author

yeah...

@git-developer
Copy link
Contributor

The DDM4000 mapping has the concept of an Active layer, i.e. a ComponentContainer which is currently in effect. It is set to either the Default layer or the Shift layer. On startup, the Active layer is the Default layer. While the Shift button is pressed, the Active layer is changed to the Shift layer. When the Shift button is released, the Active layer is changed back to the Default layer.

I observe that this works as expected using _.merge(). When using deepMerge(), the Active layer differs. I will have to investigate further.

@git-developer
Copy link
Contributor

Some more details:

  • In addition to id, a few more read-only properties (functions) are affected.
  • The merge result differs. Looks as if the merge has no or partial effect. For example, reverse and reverseroll are swapped by the layer shift. The lodash result contains reverseroll (correct), the deepMerge result contains reverse (wrong). Both results are attached (logged via printObject(), transformed to valid JSON with properties sorted).

Lodash

Content of active layer after unshift:
activeLayer-after-unshift.lodash.json

Log:

07:28:42.105 Debug [Controller] [DEBUG] LayerManager.unshift()
07:28:42.105 Debug [Controller] "Disconnected ([Channel1], loop_enabled) from connection {874f9e73-285b-4d70-9c63-55a9329fb0cb}"
07:28:42.106 Debug [Controller] "Disconnected ([QuickEffectRack1_[Channel1]_Effect1], button_parameter1) from connection {5f3b3476-4a11-411c-8653-62bd29e6de69}"
07:28:42.106 Debug [Controller] "Disconnected ([Channel2], loop_enabled) from connection {589a79e9-dbe9-48c7-8abf-54ca4de73a1f}"
07:28:42.106 Debug [Controller] "Disconnected ([QuickEffectRack1_[Channel2]_Effect1], button_parameter1) from connection {62ec3583-092c-4291-b3ea-d0aa95e1c2ad}"
07:28:42.106 Debug [Controller] "Connected ([QuickEffectRack1_[Channel1]_Effect1], enabled) to connection {20c0cf1f-d425-4dfd-a993-b83386a3e8ff}"
07:28:42.106 Debug [Controller] "outgoing: " "MIDI 1 x 1 MIDI 1:  status 0x90 (ch 1, opcode 0x9), ctrl 0x02, val 0x00"
07:28:42.106 Debug [Controller] "Connected ([QuickEffectRack1_[Channel2]_Effect1], enabled) to connection {6519bbca-f436-41a2-87f5-733bc7765c31}"
07:28:42.107 Debug [Controller] "outgoing: " "MIDI 1 x 1 MIDI 1:  status 0x90 (ch 1, opcode 0x9), ctrl 0x0E, val 0x00"

deepMerge

Content of active layer after unshift:
activeLayer-after-unshift.gh14206.json

Log:

07:26:42.256 Debug [Controller] [DEBUG] LayerManager.unshift()
07:26:42.257 Debug [Controller] "Disconnected ([Channel1], loop_enabled) from connection {47dff631-9c87-4d8a-a863-3548de98c8d9}"
07:26:42.257 Debug [Controller] "Disconnected ([QuickEffectRack1_[Channel1]_Effect1], button_parameter1) from connection {4b35d9ed-15b5-4fe0-a7b1-46861c669884}"
07:26:42.257 Debug [Controller] "Disconnected ([Channel2], loop_enabled) from connection {68e8d3f2-3533-444b-8e63-5b3a8d81071c}"
07:26:42.257 Debug [Controller] "Disconnected ([QuickEffectRack1_[Channel2]_Effect1], button_parameter1) from connection {c55dfa7c-dde1-4278-bb14-2817b8b8b8bf}"
07:26:42.259 Warning [Controller] Cannot assign to read-only property "id"
07:26:42.260 Warning [Controller] Cannot assign to read-only property "isConnected"
07:26:42.260 Warning [Controller] Cannot assign to read-only property "objectNameChanged"
07:26:42.260 Warning [Controller] Cannot assign to read-only property "disconnect"
07:26:42.260 Warning [Controller] Cannot assign to read-only property "trigger"
07:26:42.260 Debug [Controller] "Connected ([Channel1], loop_enabled) to connection {e42c91c0-25ce-4405-ac16-ecc951b0be94}"
07:26:42.261 Debug [Controller] "outgoing: " "MIDI 1 x 1 MIDI 1:  status 0x90 (ch 1, opcode 0x9), ctrl 0x00, val 0x00"
07:26:42.261 Debug [Controller] "Connected ([QuickEffectRack1_[Channel1]_Effect1], button_parameter1) to connection {a3c4d7cb-77a6-4f92-b07b-b1728f223568}"
07:26:42.261 Debug [Controller] "outgoing: " "MIDI 1 x 1 MIDI 1:  status 0x90 (ch 1, opcode 0x9), ctrl 0x02, val 0x00"
07:26:42.261 Debug [Controller] "Connected ([Channel2], loop_enabled) to connection {1463066c-9bde-4e72-b118-3fd575b6a6a5}"
07:26:42.261 Debug [Controller] "outgoing: " "MIDI 1 x 1 MIDI 1:  status 0x90 (ch 1, opcode 0x9), ctrl 0x0C, val 0x00"
07:26:42.261 Debug [Controller] "Connected ([QuickEffectRack1_[Channel2]_Effect1], enabled) to connection {1f91d7df-5877-4fd2-9ca3-9018609d0709}"
07:26:42.261 Debug [Controller] "outgoing: " "MIDI 1 x 1 MIDI 1:  status 0x90 (ch 1, opcode 0x9), ctrl 0x0E, val 0x00"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants