TheQwertiest / foo_spider_monkey_panel

foobar2000 component that allows to use JavaScript to create CUI/DUI panels
https://theqwertiest.github.io/foo_spider_monkey_panel/
MIT License
274 stars 21 forks source link

Lock types not retrieved properly in some cases #183

Open regorxxx opened 2 years ago

regorxxx commented 2 years ago

Steps:

const plsName = 'Query'; const index = plman.FindPlaylist(plsName); const types = index !== -1 ? [...new Set(plman.GetPlaylistLockedActions(index))] : []; const name = index !== -1 ? plman.GetPlaylistLockName(index) : ''; const isSMPLock = name === 'foo_spider_monkey_panel' || !name; const isLocked = types.length ? true : false; console.log({isLocked , isSMPLock, name, types, index});

console.log(plman.IsPlaylistLocked(index));



> [09:02:13] Object {isLocked=false, isSMPLock=false, name="Quicksearch", types=[], index=19}
> [09:02:13] true

So deprecated method tells it's a locked playlist while new one says the opposite.
Playlist can not be renamed. (the other actions seem to work)
marc2k3 commented 2 years ago

There is no bug here. Seems like quicksearch is being weird by installing a lock but returning false for every lock type.

Even if you don't know C++, you can guess what bool means on my implementation here

https://github.com/marc2k3/foo_jscript_panel/blob/7de08446cbf92fa576a6f39ced6c0b270d492359/src/FB2K/PlaylistLock.h#L58L91

And it must be returning 0 for the get_filter_mask method too (that's all of them combined). SMP will be using this to build the array of values you're using.

As for the playlist rename thing, not a clue.

regorxxx commented 2 years ago

Then that's a bug on quicksearch on my book XD Anyway the playlist is clearly locked for renaming, not sure how that was implemented but it's clearly wrong.

Btw the playlist appears with a lock on the status bar.

image

Just give u the info, it may well be that quicksearch should be fixed to properly use locks, but this may not be the only plugin or use-case where playlist are locked and not using lock types. So maybe the old deprecated method still has some use (?)

marc2k3 commented 2 years ago

It was stupid oclock earlier and all those methods must be returning true as they're the allowable actions, not false like I incorrectly said.

And the internal method used by IsPlaylistLocked isn't deprecated in the SDK. It's only the SMP version marked as deprecated because it's not particularly useful for end users to know a lock is in place without know the kinds of lock.

I guess the real purpose of it is for developers (and script authors) determining whether you can apply your own lock. A simple true/false without needing to know about the types of lock in place - just that it is locked.

regorxxx commented 2 years ago

(I was talking only about SMP) Well I understood marking it as deprecated when it's clearly needed made no sense. So it was either a design error or a bug. I also understood deprecated methods were going to be deleted at some point too, but as we can see now, this one is still needed. In fact now we need both.

Documentation should reflect this, since it recommends to not use it anymore and just replace it with lock types, which would clearly induce to bugs in some use-cases. Method should not be deprecated and documentation should advice to use both whenever lock types are used, since that method is omitting some cases. https://theqwertiest.github.io/foo_spider_monkey_panel/assets/generated_files/docs/html/plman.html#.IsPlaylistLocked

marc2k3 commented 2 years ago

If a component applies a lock, the return value from its get_filter_mask mask implementation should never be zero. quicksearch is behaving very badly indeed.

So yes, the method shouldn't be deprecated to account for other component developer's poor design choices. Or it could be a simple mistake? I've made plenty of those.