Review of attachment 292425 [details] [review]:

It is better to lock down the "favorite-apps" key in dconf instead of adding another GSettings key. That way, it would only be necessary to check whether the existing key was writable (with g_settings_is_writable) before showing the menu items or allowing DND, rather than checking whether the new key was disabled. Using a dconf lock has the added benefit that it prevents the user from changing the value, such as by using dconf-editor or the gsettings tool, of the locked key.

::: data/org.gnome.shell.gschema.xml.in.in
@@ +73,3 @@
     </key>
+    <key name="lockdown-disable-change-favorites" type="b">
+      <default>true</default>

Should not be enabled by default.

Thank you. I had forgotten that dconf (compared to gconf) allows keys to be locked.

However, maybe you can help me figure out how to lock a dconf key in general. dconf-editor doesn't offer that.

I can't get it work by adding a file in either /etc/dconf/db/local.d/locks/ or /etc/dconf/db/site.d/locks as suggested here:
https://wiki.gnome.org/Projects/dconf/SystemAdministrators
After doing that, I can still change my background picture. 

Note that I had to create those locks and site.d directories first because my system (Ubuntu 14.10) didn't have them.

gconf allows keys to be locked with a mandatory (read-only) settings source:

https://help.gnome.org/admin/system-admin-guide/2.32/gconf-24.html.en#gconf-26

Unlike dconf, the key and value are stored together.

I followed the instructions at that link, and when trying to write to the key (with the gsettings tool), the write fails with the error "The key is not writable". Check that you followed the instructions correctly, especially the part about specifying a profile (and system-db in particular).

Thanks again. And I'm sorry for derailing this bug report, but I think the information is very useful.

So, at least on Ubuntu (14.10) and Fedora (21) there is no /etc/dconf/profiles/user, so dconf defaults to a hard-coded default profile that doesn't use either /etc/dconf/db/local.d/ or /etc/dconf/db/site.d/:
https://wiki.gnome.org/Projects/dconf/SystemAdministrators#Hard-wired_Default

If I create /etc/dconf/profiles/user and give it a fuller specification such as
  https://wiki.gnome.org/Projects/dconf/SystemAdministrators#Profile_Specification
then my lock files, for instance in /etc/dconf/db/local.d/00_mylocks, do work.

Do you know if a lock file can go somewhere in ~/dconf/. I know the knowledgeable user could then unlock it. Or would we always need to create that /etc/dconf/profile/user file? Did you need to create that file?

Created attachment 292551 [details] [review]
give better feedback when the favorite-apps key is not writable

Heading out now, but the attached patch works for me.

Yes, that works great. Thanks.

I wonder if we should allow the drag to even start. Nothing else seems to accept the drop.

I'm still interested in my questions in comment #4 about how you added the lock.

Comment on attachment 292551 [details] [review]
give better feedback when the favorite-apps key is not writable

Marking as needs-work as the drag needs to be disabled if the key is not writable.

(In reply to comment #6)
> I wonder if we should allow the drag to even start. Nothing else seems to
> accept the drop.

Both the window picker and workspace switcher accept drops of launchers, so at least the dash should still allow starting the drag. In case of the app picker, window picker and workspace switcher are hidden, so it might make sense to disable DND in that case (though in multihead setups, non-primary monitors are still valid drag targets) ...

/etc should be completely empty on a system that is configured to default settings.

That means that a missing /etc/dconf is just an indication that dconf is functioning as per defaults.  If you want to change that, you can create /etc/dconf and populate it according to your needs.  This is the intended way to approach this problem.

There is not support for putting profile information in ~ unless you use the DCONF_PROFILE environment variable to point to a file there.

Created attachment 295596 [details] [review]
0001-App-Picker-Prevent-drag-if-favorite-apps-is-locked.patch

> In case of the app picker, window picker and workspace switcher are hidden, so 
> it might make sense to disable DND in that case (though in multihead setups,
> non-primary monitors are still valid drag targets) ...

This patch disables the drag for the Favorites and All app pickers, but not for the Dash. I've done the is_writable() checking outside of the AppIcon to avoid repeated calls.

Review of attachment 295596 [details] [review]:

Apart from the comments below, the commit message should point out the reasoning behind the change instead of paraphrasing the code

::: js/ui/appDisplay.js
@@ +521,3 @@
 
+        // Allow dragging of the icon only if the Dash would accept a drop to
+        // change favorite-apps. There are no other possible drop targets from

Not true, on multi-monitor systems, non-primary monitors are possible drop targets. Drag-to-launch is a bit of an Easter egg and even more so in this case, so I don't think it should stop us from disabling DND when favorites are locked, but the comment should still be accurate.

@@ +529,3 @@
+
+            let icon = new AppIcon(app,
+                                   {notDraggable: favoritesLocked});

Style nit: missing space after '{' and before '}'

@@ +1565,3 @@
         this._menuManager = new PopupMenu.PopupMenuManager(this);
 
+        if (!this._notDraggable) {

this._notDraggable is uninitialized, making this condition always true (also: "not not draggable" - eeeks)

::: js/ui/dash.js
@@ +536,3 @@
                                              { setSizeManually: true,
                                                showLabel: false });
+        if (appIcon._draggable) {

The app icon is created draggable just above, so not sure the condition adds anything

::: js/ui/iconGrid.js
@@ +42,3 @@
                                         setSizeManually: false,
+                                        showLabel: true,
+                                        notDraggable: false });

This is a part I genuinely dislike - you are adding a property to a base class that is only implemented in a subclass.
Either add a 2nd params argument to AppIcon, or do something like:

  // handle our own parameters before passing them to BaseIcon
  let iconParams = Params.parse(params, { draggable: true }, true);
  let draggable = iconParams['draggable'];
  delete iconParams['draggable'];

  this.icon = new BaseIcon();

Created attachment 295622 [details] [review]
0001-App-Picker-Prevent-drag-if-favorite-apps-is-locked-v2.patch

(In reply to comment #11)

> Review of attachment 295596 [details] [review]:
> 
> Apart from the comments below, the commit message should point out the
> reasoning behind the change instead of paraphrasing the code

I've added an extra explanatory line. I find the detailed change description useful as a check of my own code and for future reference, but I'd happily remove it.

[snip]
> +        if (!this._notDraggable) {
> 
> this._notDraggable is uninitialized, making this condition always true
_
Sorry, I had changed from this._notDraggable to notDraggable and didn't correct that.

> (also:
> "not not draggable" - eeeks)

Sorry again, I had not yet seen that I could specify a true default.

> ::: js/ui/dash.js
> @@ +536,3 @@
>                                               { setSizeManually: true,
>                                                 showLabel: false });
> +        if (appIcon._draggable) {
> 
> The app icon is created draggable just above, so not sure the condition adds
> anything

It just seemed wise to check now that we've made it possible for _draggable to be uninitizialized.


> ::: js/ui/iconGrid.js
> @@ +42,3 @@
>                                          setSizeManually: false,
> +                                        showLabel: true,
> +                                        notDraggable: false });
> 
> This is a part I genuinely dislike - you are adding a property to a base class
> that is only implemented in a subclass.
> Either add a 2nd params argument to AppIcon, or do something like:
> 
>   // handle our own parameters before passing them to BaseIcon
>   let iconParams = Params.parse(params, { draggable: true }, true);
>   let draggable = iconParams['draggable'];
>   delete iconParams['draggable'];
> 
>   this.icon = new BaseIcon();

I went with the second choice, to avoid needing any changes in other code that creates an AppIcon.

How's this updated patch? Many thanks for taking the time to help me to get it right.

Review of attachment 295622 [details] [review]:

(In reply to comment #12)
> I've added an extra explanatory line. I find the detailed change description
> useful as a check of my own code and for future reference

OK with me, if you update it for the latest code changes (there are still references to notDraggable in the commit message). Code looks good to me now.

Thanks. So I can push it with that commit? (and put Javascript on my CV).

Sorry, comment #13 was meant as "please fix the commit message and push", so yes :-)

I'm wanting to re-order the fav icons on the dock... and I can left-click/drag an icon to pick it up... but it isn't letting me drop it.  Well, I can drop it but it just disappears.  In the past I think if I hovered the icon in a reasonable position for a drop it would give a visual indicator as to where it would land if I let go... which allowed me to re-order the icons.  Is this no longer working because of this bug and the fixes for it?  Which is to say has the ability to re-order icons on the dock been removed by default... or is this unrelated and I need to file a new bug?  Or it isn't a bug but a feature as you don't want users to be able to re-order icons on the dock?

Oh, I forgot to mention I'm testing GNOME 3.15.90 on Fedora 22 (pre) Alpha.

Adding/removing/reordering favorites is expected to work as before, *except* when the setting has been locked down (read: the user cannot change the favorites list, and any DND action would not have any effect anyway). So yes, please file a new bug ...