[vlc-devel] [PATCH 16/18] gui/qt: fix #17184 (missing entries in context-menu)

[Filip Roséen]

> On 2016-07-21 18:11, Francois Cartegnie wrote:
>> Le 20/07/2016 à 12:00, Filip Roséen a écrit :
>> 
>>> -    const PLItem *item = getItem( index );
>>> +    bool const b_readonly = THEPL->p_root->i_flags & 
>>> PLAYLIST_RO_FLAG;
>>> +    AbstractPLItem * item = VLCModel::getItem( index );
>> 
>> That's incorrect. Breaks everything on RO items.
>> 
>> - The playlist root is never RO.
>> - The item itself can be RO in a non RO node.
> 
> But for item specific entities,  "item->readOnly()" is checked? The
> b_readonly is only used for non-item specific entities,
> but if p_root is /never/ read-only that variable (and checks that use
> them) sure is redundant and should be removed.

Please consider this patch obsolete
-----------------------------------

Given that this issue/patch has already generated a lot of noise on 
vlc-devel,
consider it obsolete. I will re-submit a new patch-batch where the 
relevant
changes are split up into smaller chunks to make it easier to reason 
about.

Currently I think it might be best to;

   - split logic changes and "beautification" into different patches,
   - remove redundant checks (given what you wrote the current 
implementation
     suffers from this problem too), and;
   - fix one action-logic at a time (one case = one patch).

I see no point in me arguing for something if the whole approach is as
insufficient as you make it sound. I have tested the patch and it, as 
far as
I can see, fixes the bug in question - though I could be wrong, but this
discussion is not very constructive.

I would rather have something we can discuss in a straight-forward 
matter than
something that just leads to confusion and misunderstanding.

Thank you for all the feedback, it is as always very much appreciated!

Regarding the associated issue (#17184)
--------------------------------------

I have created a local file "ticket_17184/NOTES" containing all the 
information
from this thread. If someone else will work on the issue, please let me 
know to
prevent N people working to solve the same problem (causing N-1 people 
to work
for something that might already have a, soon to be pushed, solution).

See the below for more information:

   - https://trac.videolan.org/vlc/ticket/17184


Something that could use some more attention (to fix #10051)
------------------------------------------------------------

It seems like this discussion has also taken focus away from something 
that
should be trivial to merge, more specifically the patch linked below:

  - 
https://mailman.videolan.org/pipermail/vlc-devel/2016-July/108648.html