Bug 468158

Comment 1 Alexander Nyßen 2015-05-25 07:23:07 EDT

Fixed that not only .dot files can be opened in DOT Graph view (added *.* as second file extension). Ensured that a DotExtractor is used to load file contents. Adjusted the messages to indicate that loading embedded content is possible. 

All changes pushed to origin/master: http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=a135724934370f81fadd192a909fcc58ab05b2ab

Resolving as fixed in 3.10.0 RC2.

Comment 2 Alexander Nyßen 2015-05-25 10:39:51 EDT

Changed that DotExtractor is only used if not opening .dot files (so the current behavior for .dot files is preserved):

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=88247912c39a5d5b423352f18aa1fcb1914fec36

Comment 3 Alexander Nyßen 2015-05-25 11:14:52 EDT

And made it even more tight when evaluating whether a dot file loaded:

http://git.eclipse.org/c/gef/org.eclipse.gef4.git/commit/?id=9ffd38e8fd4859c15679a210eb5eafe8b426efa0

Comment 4 Fabian Steeg 2015-05-25 16:10:25 EDT

I think the actual issue here is the documentation. When I moved over the graph view functionality from the old Zest bundles, I dropped the embedded DOT support. Looks like I forgot to update the documentation and left the DotExtractor class in place (sorry!), so these fixes are quite reasonable.

But while with these fixes embedded DOT can now be loaded again, the sync mode still doesn't support other files. The visitor would have to consider every file touched in the workspace, which seemed like an unwarranted overhead for the few cases where a file might actually contain embedded DOT.

Since the current state is inconsistent (load both *.dot and *.* files, but only *.dot files will then work in synced mode), and full support for embedded DOT is not feasible for Mars, I'd propose we roll back these changes and update the documentation instead.

Comment 5 Alexander Nyßen 2015-05-25 16:39:45 EDT

(In reply to Fabian Steeg from comment #4)
> I think the actual issue here is the documentation. When I moved over the
> graph view functionality from the old Zest bundles, I dropped the embedded
> DOT support. Looks like I forgot to update the documentation and left the
> DotExtractor class in place (sorry!), so these fixes are quite reasonable.
> 
> But while with these fixes embedded DOT can now be loaded again, the sync
> mode still doesn't support other files. The visitor would have to consider
> every file touched in the workspace, which seemed like an unwarranted
> overhead for the few cases where a file might actually contain embedded DOT.
> 
> Since the current state is inconsistent (load both *.dot and *.* files, but
> only *.dot files will then work in synced mode), and full support for
> embedded DOT is not feasible for Mars, I'd propose we roll back these
> changes and update the documentation instead.

I don't think its inconsistent to have sync-mode only work for our own DOT editor, i.e. for .dot files. As long as its clearly documented (and this is now the case) its something the user might well live with. As we would otherwise loose that nice functionality, let's leave it in. 

In the future, we can further enhance the sync-mode to also support embedded DOT contents as far as this is possible. Let me mention that we have other sync-mode issues open as well, e.g. that the live contents of the DOT editor is evaluated (see bug #337644).

Comment 6 Fabian Steeg 2015-05-25 18:01:36 EDT

From my experience with the embedded DOT support I'm not sure it's actually a good idea. I think we should get really good at the clear use case of visualizing *.dot files before we add new use cases. Because often these are more complex than it seems, and things look broken. For instance, what if we have multiple embedded graphs in a file? It's basically the same line of reasoning as in my comments on bug 337644 (which you mentioned): to focus on doing a single thing right, in particular for our first release.

Comment 7 Alexander Nyßen 2015-05-26 03:34:43 EDT

(In reply to Fabian Steeg from comment #6)
> From my experience with the embedded DOT support I'm not sure it's actually
> a good idea. I think we should get really good at the clear use case of
> visualizing *.dot files before we add new use cases. Because often these are
> more complex than it seems, and things look broken. For instance, what if we
> have multiple embedded graphs in a file? It's basically the same line of
> reasoning as in my comments on bug 337644 (which you mentioned): to focus on
> doing a single thing right, in particular for our first release.

I think we are a bit too late in the release cycle to start this detailed discussion now. I have restored what has long been there and always been documented, so this should be fine for Mars.

I agree that there are several open issues for the (standalone) DOT rendering as well, and that these should get priority in treatment. Let's open dedicated bugs to track all the inconsistencies we see with what is currently there, so the shortcomings are clearly documented. We can then have a dedicated discussion and planning after Mars. If we decide to drop the entire embedded rendering functionality, we can announce this early and give the community a chance to speak up in case this functionality seems to be worth preserving.

We will only release a first snapshot with Mars (where all API is still provisional), and I hope there will be several after, giving us lots of opportunity for improvement. We may investigate this even for Mars SR1 if we decide to ship another minor release instead of a service one (I will raise this question on the mailing list soon).

Comment 8 Fabian Steeg 2015-05-26 06:29:00 EDT

(In reply to Alexander Nyßen from comment #7)

> I think we are a bit too late in the release cycle to start this detailed
> discussion now.

I completely agree about that, which is why I proposed to roll back to what was in all our Mars milestones and RC1, instead of effectively adding half baked functionality in RC2.

Comment 9 Alexander Nyßen 2015-05-26 10:54:04 EDT

(In reply to Fabian Steeg from comment #8)
> (In reply to Alexander Nyßen from comment #7)
> 
> > I think we are a bit too late in the release cycle to start this detailed
> > discussion now.
> 
> I completely agree about that, which is why I proposed to roll back to what
> was in all our Mars milestones and RC1, instead of effectively adding half
> baked functionality in RC2.

Fabian, I understand your concern, but I do not share it. This functionality is not more nor less "half baked" than anything else we currently provide w.r.t. DOT support, which - as a whole - is still lacking a lot of desirable functionality. Let's offer it to the community and take the chance to get feedback on it (as with all other parts of DOT support). If the community doesn't like it or does not use it at all, we may drop it after Mars. If not, we might get valuable feedback on how to further improve it. 

In either case, there is no harm to be feared, as this functionality does not affect anybody who does not explicitly choose to load a file with embedded DOT contents. Otherwise I would not have included it in RC2.