Behavior for read-only maps (#2202)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Behavior for read-only maps (#2202)

Felix Natter
Administrator
Dear Devs,

I'd like to discuss a question related to
http://sourceforge.net/p/freeplane/bugs/2202/:

The current state is:

- for read-only maps, the save action is simply disabled
  and thus it is a no-op (which is confusing!)

- for read-only maps that are documentation maps,
  any changes are ignored (no query "do you want to save?"):

        @Override
    public boolean isSaved() {
            return super.isSaved() || containsExtension(DocuMapAttribute.class);
    }
  [the code above is not clean --> I moved the containsExtension(...)
   check to isReadOnly()]

- for other read-only maps (i.e. chmod -w *.mm), the user is asked whether
  he/she wants to save the map (if changed), but when he/she says yes,
  and overwrites the file, the saving is not executed (the file is not
  saved)

I'd like to change it like this (simple partial patch is attached):

- for read-only maps (no matter whether documentation map or chmod -w
  *.mm), the save action remains enabled, but you get a warning popup
  "Cannot save: This map is read-only."
  (to avoid that users hit Ctrl+s and expect that the map is saved!)
  [we could also consider showing the save _as_ dialog in this case]

- when read-only maps are changed, they are marked as modified (*)
  (keep this behavior), but when exiting freeplane/closing the map,
  the save _as_ dialog is shown

- Report an error when trying to overwrite a read-only *.mm with the
  current map instead of silently failing.

What do you think about this?

Thanks and Best Regards,
--
Felix Natter

diff --git a/freeplane/src/org/freeplane/features/map/MapModel.java b/freeplane/src/org/freeplane/features/map/MapModel.java
index 24cc46c..5c514dc 100644
--- a/freeplane/src/org/freeplane/features/map/MapModel.java
+++ b/freeplane/src/org/freeplane/features/map/MapModel.java
@@ -36,6 +36,7 @@ import org.freeplane.core.util.TextUtils;
 import org.freeplane.features.filter.Filter;
 import org.freeplane.features.filter.FilterController;
 import org.freeplane.features.icon.IconRegistry;
+import org.freeplane.features.map.mindmapmode.DocuMapAttribute;
 import org.freeplane.features.mode.Controller;
 import org.freeplane.features.mode.ModeController;
 
@@ -187,7 +188,7 @@ public class MapModel {
  }
 
  public boolean isReadOnly() {
- return readOnly;
+ return readOnly || containsExtension(DocuMapAttribute.class);
  }
 
  public boolean isSaved() {
diff --git a/freeplane/src/org/freeplane/features/map/mindmapmode/MMapModel.java b/freeplane/src/org/freeplane/features/map/mindmapmode/MMapModel.java
index 5a93b5a..48f4cca 100644
--- a/freeplane/src/org/freeplane/features/map/mindmapmode/MMapModel.java
+++ b/freeplane/src/org/freeplane/features/map/mindmapmode/MMapModel.java
@@ -60,7 +60,7 @@ public class MMapModel extends MapModel {
 
  @Override
     public boolean isSaved() {
-    return super.isSaved() || containsExtension(DocuMapAttribute.class);
+    return super.isSaved();
     }
 
  /**
diff --git a/freeplane/src/org/freeplane/features/url/mindmapmode/SaveAction.java b/freeplane/src/org/freeplane/features/url/mindmapmode/SaveAction.java
index fd53d36..d5f0d56 100644
--- a/freeplane/src/org/freeplane/features/url/mindmapmode/SaveAction.java
+++ b/freeplane/src/org/freeplane/features/url/mindmapmode/SaveAction.java
@@ -21,9 +21,11 @@ package org.freeplane.features.url.mindmapmode;
 
 import java.awt.event.ActionEvent;
 
-import org.freeplane.core.ui.AFreeplaneAction;
+import javax.swing.JOptionPane;
 
+import org.freeplane.core.ui.AFreeplaneAction;
 import org.freeplane.core.ui.EnabledAction;
+import org.freeplane.core.util.LogUtils;
 import org.freeplane.core.util.TextUtils;
 import org.freeplane.features.map.MapModel;
 import org.freeplane.features.mode.Controller;
@@ -42,6 +44,14 @@ class SaveAction extends AFreeplaneAction {
  }
 
  public void actionPerformed(final ActionEvent e) {
+ if (Controller.getCurrentController().getMap().isReadOnly())
+ {
+ JOptionPane.showMessageDialog(null,
+    "This map is read-only.",
+    "Attempt to save",
+    JOptionPane.WARNING_MESSAGE);
+ return;
+ }
  final boolean success = ((MModeController) Controller.getCurrentModeController()).save();
  final Controller controller = Controller.getCurrentController();
  if (success) {
@@ -57,6 +67,6 @@ class SaveAction extends AFreeplaneAction {
  public void setEnabled() {
  final Controller controller = Controller.getCurrentController();
  MapModel map = controller.getMap();
- setEnabled(map != null && ! map.isSaved() && ! map.isReadOnly());
+ setEnabled(map != null && ! map.isSaved());
  }
 }
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Behavior for read-only maps (#2202)

Felix Natter
Administrator
hello devs,

I made the changes as described above:
  https://github.com/freeplane/freeplane/commit/e53e5540a44e174cfae8be80121e642ff2bebbbd
  https://github.com/freeplane/freeplane/commit/c23dfceeb9b60d98534ade2c387a3a6afc3b71eb
and I need your input on this:

- the behavior of org.freeplane.features.map.mindmapmode.MMapModel.isSaved()
has changed from

  return super.isSaved() || containsExtension(DocuMapAttribute.class);
to
  return super.isSaved();
[and containsExtension(DocuMapAttribute.class) is honored in isReadOnly()]
(yes, I'll remove the unncessary override)

So what should the behavior in the scripting API be like?

- org.freeplane.plugin.script.proxy.MapProxy.close(boolean, boolean)
=> I suggest to leave it as-is (will not close modified docu maps without force==true)

- org.freeplane.plugin.script.proxy.MapProxy.isSaved():
=> I suggest to leave it as-is (do not consider modified docu maps as saved)

- org.freeplane.plugin.script.proxy.MapProxy.save(boolean):
=> not sure about this one

Other use:

- org.freeplane.features.url.mindmapmode.MFileManager.save(MapModel)
=> if (map == null || map.isSaved() || map.isReadOnly()) ?

Thanks and Best Regards,

Felix
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Behavior for read-only maps (#2202)

Volker Börchers
Administrator
Hi Felix,

I think that your change of MMapModel.isSaved() is a good one and that
the scripting API should just benefit from it. So no change is required
there.

The MFileManager should probably be changed as this checks seem to be
better placed in the action (where it is, see SaveAction.setEnabled()).

Regards, Volker

Am 09.12.2014 um 15:38 schrieb Felix Natter [via Freeplane Developer]:

> hello devs,
>
> I made the changes as described above:
> https://github.com/freeplane/freeplane/commit/e53e5540a44e174cfae8be80121e642ff2bebbbd
> https://github.com/freeplane/freeplane/commit/c23dfceeb9b60d98534ade2c387a3a6afc3b71eb
> and I need your input on this:
>
> - the behavior of
> org.freeplane.features.map.mindmapmode.MMapModel.isSaved()
> has changed from
>
>    return super.isSaved() || containsExtension(DocuMapAttribute.class);
> to
>    return super.isSaved();
> [and containsExtension(DocuMapAttribute.class) is honored in isReadOnly()]
> (yes, I'll remove the unncessary override)
>
> So what should the behavior in the scripting API be like?
>
> - org.freeplane.plugin.script.proxy.MapProxy.close(boolean, boolean)
> => I suggest to leave it as-is (will not close modified docu maps
> without force==true)
>
> - org.freeplane.plugin.script.proxy.MapProxy.isSaved():
> => I suggest to leave it as-is (do not consider modified docu maps as
> saved)
>
> - org.freeplane.plugin.script.proxy.MapProxy.save(boolean):
> => not sure about this one
>
> Other use:
>
> - org.freeplane.features.url.mindmapmode.MFileManager.save(MapModel)
> => if (map == null || map.isSaved() || map.isReadOnly()) ?
>
> Thanks and Best Regards,
>
> Felix
>
>
> ------------------------------------------------------------------------
> If you reply to this email, your message will be added to the discussion
> below:
> http://freeplane-developer.996965.n3.nabble.com/Behavior-for-read-only-maps-2202-tp583p585.html
>
> To start a new topic under Freeplane Developer, email
> [hidden email]
> To unsubscribe from Freeplane Developer, click here
> <
> NAML
> <
http://freeplane-developer.996965.n3.nabble.com/template/NamlServlet.jtp?macro=macro_viewer&id=instant_html%21nabble%3Aemail.naml&base=nabble.naml.namespaces.BasicNamespace-nabble.view.web.template.NabbleNamespace-nabble.view.web.template.NodeNamespace&breadcrumbs=notify_subscribers%21nabble%3Aemail.naml-instant_emails%21nabble%3Aemail.naml-send_instant_email%21nabble%3Aemail.naml>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Behavior for read-only maps (#2202)

Felix Natter
Administrator
"Volker Börchers [via Freeplane Developer]"
<[hidden email]> writes:

> Hi Felix,
>
> I think that your change of MMapModel.isSaved() is a good one and that
> the scripting API should just benefit from it. So no change is required
> there.

hello Devs,

I had to make one more change:
  org.freeplane.features.map.mindmapmode.MMapController.newDocumentationMap(URL)

add:
  // avoid that new docu maps are considered modified
  newModel.setSaved(true);

@Dimitry: this works, but is there another place where docu maps are
created?
maybe org.freeplane.features.link.mindmapmode.MLinkController.loadURL(NodeModel,
ActionEvent) ?

@All: Would someone want to review or even test this feature?
  https://github.com/freeplane/freeplane/commits/readonly_maps
(it's a fix for #2202, sf is currently down)

Cheers and Best Regards,
--
Felix Natter
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Behavior for read-only maps (#2202)

Dimitry Polivaev
Administrator
> @Dimitry: this works, but is there another place where docu maps are
> created?
> maybe org.freeplane.features.link.mindmapmode.MLinkController.loadURL(NodeModel,
> ActionEvent) ?

I looked at the code. All maps with DocuMapAttribute are currently created only by
MMapController.newDocumentationMap(URL)


MLinkController.loadURL just temporarily adds DocuMapAttribute.instance as an extension of
modeController to force call to newDocumentationMap from super.loadURL

Regards,
Dimitry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Behavior for read-only maps (#2202)

Felix Natter
Administrator
hi,

Blair was so kind to test the svn use case of #2202 and I tested the
rest, so readonly_maps has been merged to 1.4.x.

Best Regards,
--
Felix Natter
Loading...