Re: [PATCH 2/7] WPrefs: Edit proplist menus referenced by WMRootMenu.

On 08/19/2017 07:52 AM, Carlos R. Mafra wrote:
> On Sat, 19 Aug 2017 at  7:14:03 -0400, Doug Torrance wrote:
>> On 08/19/2017 06:13 AM, Carlos R. Mafra wrote:
>>> Thanks Doug.
>>>
>>> I have one comment:
>>>
>>> On Fri, 18 Aug 2017 at 20:37:46 -0400, Doug Torrance wrote:
>>>> Previously, WPrefs could only be used to edit the menu specified in
>>>> WMRootMenu.
>>>>
>>>> In a recent commit, the ability to specify a menu in proplist format defined
>>>> in another file which is referenced by WMRootMenu was added.  However, if a
>>>> user attempted to edit such a menu in WPrefs, an error dialog appeared.
>>>>
>>>> We add the ability for WPrefs to read such a menu.
>>>
>>> That's nice!
>>>
>>>> After the user makes any changes, the result is stored in WMRootMenu,
>>>> and *not* the original file.
>>>
>>> This is unexpected and will probably defeat the purpose of
>>> having a separate file for the menu in the first place.
>>>
>>> What is the technical challenge to save the changes back
>>> into the original file? That would be much better.
>>
>> There is a chance that the original file is read-only.
> 
> Then why a user would try to modify it using WPrefs?

They might want to use a read-only file as a starting point to customize 
their own menu.

>> For example, the default menu in the Debian package is stored in
>> /usr/share/WindowMaker/menu.hook (which is actually a symbolic link to
>> /etc/GNUstep/Defaults/plmenu.Debian), and WMRootMenu consists of the single
>> line:
>> "menu.hook"
>>
>> As of the lastest version of the Debian package (0.95.8-2), this file is now
>> in proplist format, and so WPrefs can read it (this was my original impetus
>> for writing this patch), but it can't write back to it.
> 
> WPrefs should not do anything behind our back to bypass this
> oddity in how the Debian menu is set up. It is more desirable to
> fix Debian than to make WPrefs behave in ways users don't expect.

Is it an oddity?  Debian is just taking an advantage of a Window Maker 
feature.  In particular, if WMRootMenu contains a string enclosed in 
quotes, then assume this is a path to a menu file.

> Because a legitimate user with proplist menus in his _home_ directory
> will have a good reason to have them as separate files (otherwise
> he would not be using this feature). This user will not be happy
> to see this file merged into WMRootMenu without his permission.

That makes sense.  I just submitted a series of three patches, one of 
which writes the menu back to the original file if it that is possible. 
Another informs the user which file is being modified.

>> I suppose a solution would be to write back to the original file if
>> possible.  Otherwise, copy the modified version of the file to the user
>> directory.
> 
> No, this is not a solution.
> 
> If the user has no permission on the file then he has no right to
> expect to write to it and he should be given a "no permission" error.

The third patch in the new series does this.

> Can't you just make Debian behave better here instead? Just copy
> the menu file to the user home and use that instead. No need to
> play games with WPrefs.
> 
>> So for example, in the Debian case, after modifying the menu, we create a
>> new file, ~/GNUstep/Library/WindowMaker/menu.hook.
> 
> Why doesn't Debian write a copy of the menu.hook in
> the user dir in the first place and start from there?

That would probably work.  Debian packages generally don't monkey around 
with user home directories, but the Window Maker package does have that 
wrapper script.  Copying the menu over could be included there.

>> Since we look in the user directory before the system directory for
>> menu files, WMRootMenu will now point us to the new, modified version.
>>
>> One issue with this would be in the case that WMRootMenu references the full
>> path, e.g.,
>> "/usr/share/WindowMaker/menu.hook"
>>
>> Then we would need to also modify WMRootMenu to point to the new version.
>>
>> If this seems reasonable, I'll work on a patch.
> 
> I'd say modify your patch to write the changes back into the original
> file and let the user be given an error if he has no permission.
> 
> And then make Debian use a menu file from the user's home.
> 
> How does that sound?
> 
> Btw, I have uploaded your patches already to #next, but feel free
> to come up with a new series and I'll rebase.

Thanks for your comments, Carlos!
Doug


-- 
To unsubscribe, send mail to [email protected].