Views.Old doesn't impement documentation specs

All except GUI problems
Post Reply
User avatar
adimetrius
Posts: 68
Joined: Sun Aug 04, 2019 1:02 pm

Views.Old doesn't impement documentation specs

Post by adimetrius »

Colleagues,
Views Documentation wrote: PROCEDURE Old (ask: BOOLEAN; VAR loc: Files.Locator; VAR name: Files.Name;
VAR conv: Converters.Converter): View
conv determines the converter which is used for reading the document. conv = NIL means that no conversion is necessary, i.e., the file format already has the standard BlackBox format.
Thus, if you pass conv = NIL, then no conversion should occur. So, if you try to open a .png file with Views.Old passing conv = NIL, you should receive a NIL, because there is no Documents.Document in a png file, and thus no view can be extracted from a png file with the standard document converter.

However de-facto the implementation (in StdDialog.OldView) calls Converters.Import, passing conv to it witout change. Well,
PROCEDURE Converters.Import (loc: Files.Locator; name: Files.Name; VAR conv: Converter; OUT s: Stores.Store) wrote: If conv = NIL, the first suitable converter in list is used and returned in the VAR parameter.
Thus, Views.Old, upon receiving conv = NIL, looks for the first fitting converters and uses it to read the file. And returns a mess of characters, because png cannot be converted into a View.

How to fix:
It seems to me that something was left in confusion in StdDialog.OldView: it declares a local c: Converters.Converter, performs some manipulations with it and then never uses the result. The local c needs to be removed, and formal conv used everywhere in it's place:

Code: Select all

		c := conv;  (* remove *)
		IF c = NIL THEN c := Converters.list END;	(* use document converter *)
			(* IF conv = NIL THEN conv := Converters.list END; *)
		w := Windows.GetBySpec(loc, name, c (* -> conv *), {});
		IF w # NIL THEN s := w.doc.ThisView() END;
		IF s = NIL THEN
			Converters.Import(loc, name, conv, s);
I move to fix it.

Besides, the docu for Views.Old needs to be fixed, too, IMHO. There needs to be a mention of the case when conversion is impossible, and NIL is returned in that case. The postcondition
result = NIL
loc.res # 0
seems to be inaccurate.

Even under current implementation, a NIL may be returned if no fitting converter is found. This is low chance, since the default Blackbox has a number of 'catch all' converteres installed by default. It is one of these converters that produces the character mess - pretty useless, I would turn off their Converters.importAll flag. However, if they are not installed (in Config), a NIL may well be returned by Views.Old.
luowy
Posts: 87
Joined: Thu Dec 17, 2015 1:32 pm

Re: Views.Old doesn't impement documentation specs

Post by luowy »

Do you have an example to demonstrate your explanation?
User avatar
adimetrius
Posts: 68
Joined: Sun Aug 04, 2019 1:02 pm

Re: Views.Old doesn't impement documentation specs

Post by adimetrius »

Iuowy,

I hope this explanation helps:

Code: Select all

conv := NIL;
name := 'picture.png';
loc := Files.dir.This();
v := Views.Old(Views.dontAsk, loc, name, conv);
IF v = NIL THEN Log.String("I expect v = NIL here, because picture.png does not contain (any) View, and I passed NIL for conveter, which means, 'Dont do any conversion'")
ELSE
  WITH v: TextViews.View DO ...
  ELSE
  END
END;
...
Converters.Import(loc, name, conv, v);
(* Here, you can almost certainly assert that v # NIL, because conv = NIL means 'find the best suitable converter', and there usually is a default converter that can at least represent whatever file as text or hex *)
IF v = NIL THEN
ELSE
  WITH v: TextViews.View DO (* I expect v # NIL, although it may contain gibberish text *)
  ELSE
  END
END
luowy
Posts: 87
Joined: Thu Dec 17, 2015 1:32 pm

Re: Views.Old doesn't impement documentation specs

Post by luowy »

after play your code,

Code: Select all

MODULE ObxOldView;

	IMPORT Converters, Views, TextViews, Log := StdLog, Files;

	PROCEDURE OldView* ();
		VAR conv: Converters.Converter;
		name: ARRAY 256 OF CHAR;
		loc: Files.Locator; v: Views.View;
		
	BEGIN
		name :=(* 'picture';*)'picture.png';
		loc := Files.dir.This(""); conv := NIL;
		v := Views.Old(Views.dontAsk, loc, name, conv);
		IF v = NIL THEN 
			Log.String("I expect v = NIL here"); Log.Ln;
	    	 (* because picture.png does not contain (any) View, and I passed NIL for conveter, 
	      	   which means, 'Dont do any conversion'"*)
		ELSE
			WITH v: TextViews.View DO
				Log.String("v is a TextViews.View"); Log.Ln;
				Log.String("Converter is "+conv.imp);Log.Ln;
				Views.OpenView(v);
			ELSE
				Log.String("v is a Views.View"); Log.Ln;
				Log.String("Converter is "+conv.imp);Log.Ln;
				Views.OpenView(v);
			END
		END;

	END OldView;
	
	PROCEDURE Import* ();
		VAR conv: Converters.Converter;
		name: ARRAY 256 OF CHAR;
		loc: Files.Locator; v: Views.View;
	BEGIN
		name := (*'picture';*)'picture.png';
		loc := Files.dir.This(""); conv := NIL;
		Converters.Import(loc, name, conv, v);

		(* Here, you can almost certainly assert that v # NIL, 
			because conv = NIL means 'find the best suitable converter', 
			and there usually is a default converter that can at least represent whatever file as text or hex 
		*)

		IF v = NIL THEN 
		ELSE
			WITH v: TextViews.View DO (* I expect v # NIL, although it may contain gibberish text *)
				Log.String("v is a TextViews.View"); Log.Ln;
				Log.String("Converter is "+conv.imp);Log.Ln;
				Views.OpenView(v);
			ELSE
				Log.String("v is a Views.View"); Log.Ln;
				Log.String("Converter is "+conv.imp);Log.Ln;
				Views.OpenView(v);
			END
		END
	
	END Import;


END ObxOldView.
I see what you mean:
you expect Old return "v=NIL", when input a "alien" file with "conv = NIL", but the framework does not;
and expect Import return "v=NIL", when input a "alien" file with "conv = NIL", framework does same;

IMO, amendment no needed, please check StdApi.OpenAux to understand how to use this procedure;

P.S. loc.res = 77 mean "openFromDisk"
User avatar
adimetrius
Posts: 68
Joined: Sun Aug 04, 2019 1:02 pm

Re: Views.Old doesn't impement documentation specs

Post by adimetrius »

luowy wrote: I see what you mean:
you expect Old return "v=NIL", when input a "alien" file with "conv = NIL", but the framework does not;
and expect Import return "v=NIL", when input a "alien" file with "conv = NIL", framework does same;
Not exactly:
1) I expect Import to return "v = NIL" when input a "alian" file with "conv = NIL" ONLY if there are no suitable converters installed; the framework does NOT do the same.
2) I expect Import to return "v # NIL" when input a "alian" file with "conv = NIL" if a suitable (possibly a catch-all) converter is installed.
luowy wrote: please check StdApi.OpenAux to understand how to use this procedure;
I checked it, and I cannot see what you're hinting at; can you be more specific?

StdApi.OpenDoc/OpenAux gives wrong error reporting: it will report a file as nonexistent when in fact the file exists but there's no suitable converter for it. Here's how to check: make sure no converters are installed in Config; then

Code: Select all

MODULE VarOpener;
	
	IMPORT Views, StdApi, Log;
	
	PROCEDURE OpenDoc* (IN file: ARRAY OF CHAR);
		VAR v: Views.View;
	BEGIN
		StdApi.OpenDoc(file, v);
		IF v = NIL THEN Log.String("v = NIL"); Log.Ln
		ELSE Views.OpenView(v)
		END
	END OpenDoc;
	
END VarOpener.
Now (!)"VarOpener.OpenDoc('/tmp/3/picture.png')" logs
Log wrote: no converter found
file /tmp/3/picture.png not found
This is WRONG, the file /tmp/3/picture.png is found and doing allright.

In fact, StdApi.OpenDoc supports my second point: the docu for Views.Old is not exact and needs to be amended. Views.Old does return NIL if conversion is impossible. So I maintain my second point: the docu for Views.Old is not exact and needs to be amended.

I still maintain my first point as well: Views.Old implementation should comply with it's documentation and, when passed conv = NIL (and ask = Views.dontAsk), only read from standard BlackBox file format thru the standard document converter. This keeps things simple indeed: if you are only interested in standard BB views and documents, which is the most frequent case IMHO, then you use Views.Old; otherwise, you dig deeper and use Converters.Import.
luowy
Posts: 87
Joined: Thu Dec 17, 2015 1:32 pm

Re: Views.Old doesn't impement documentation specs

Post by luowy »

Usually the documentation is outdated than the source code;
If the documentation is incorrect and no bugs in the source code, it is better to modify the documentation rather than the source code.

This is a basic interface, many modules depend on it, changing it without checking other modules will cause a lot of trouble, and how about third-party modules?

If you are really not satisfied with this API, it is recommended to add a new function, which does no harm to the framework
Post Reply