issue-#104 calling URLs and commands (InfoCmds capabilities)

Merged to the master branch
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Ivan Denisov »

Josef Templ wrote:Dialog.IsWine() cannot be called in a parallel thread.
Even if its current implementation happens to work (by accident)
this is way too dangerous.
If somebody changes it later then the parallel thread execution may
CRASH BlackBox completely. It should be clear to anybody that this
cannot go into master. It needs to be reconsidered in more detail first.

In general, it does not give sense to sacrifice the stability of the system
for a rarely used feature.
The child thread can use "stable" memory of the parent thread, so it can not be problem with Dialog.IsWine() (Dialog.Run can not be called if Dialog is unloaded). The only problems can be if callName changes while creating a process. For that purpose I made callNameTaken variable and wait until name is taken by child thread.
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Ivan Denisov »

I made some hot fixes resulting on this discussion:
http://redmine.blackboxframework.org/pr ... 115335d51f

We can make some fixes also later on the beta stage if some side effects will be found.
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Robert »

You have made the extravagant memory usage worse!

Replace

Code: Select all

VAR callName: ARRAY 8191 OF CHAR
by

Code: Select all

VAR callName: POINTER TO ARRAY OF CHAR;
BEGIN
  NEW(callName, 32)
Then use

Code: Select all

IF LEN(fileName$) > LEN(callName) - 1 THEN NEW(callName, LEN(fileName$) + 1) END;
callName := fileName$
The 16+ kilobytes of memory will only be used if needed.
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Robert »

Ivan Denisov wrote:For error reporting you can also make modified realization which will put reports in the log window during development. Later for production version normal HostDialog.Open/Run can be used. Or you can keep your realization with reporting if it necessary for target user.
Do you mean HostDialog?

By error reporting I was not thinking of development errors, but run-time errors - for example trying to run a .exe file that is missing from your computer, or anyway Windows can't find it.
A report to the Log may not be sufficient. Your program may want to react differently depending on whether running the .exe was successful or not.

I guess we could add, later, a GetLastError procedure if we wanted to without changing the current simple Run & Open interface suggestions.
Zinn
Posts: 476
Joined: Tue Mar 25, 2014 5:56 pm
Location: Frankfurt am Main
Contact:

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Zinn »

All arguments are against this implementation are correct. But let us start once again from the beginning about the interface definition and not its implementation.

PROCEDURE Dialog.Open (IN fileName: ARRAY OF CHAR) is equivalent to the existing PROCEDURE HostTextConv.ShellExecute (path: ARRAY OF CHAR);
and
PROCEDURE Dialog.Run (IN exeName: ARRAY OF CHAR) is equivalent to PROCEDURE HostDialog.Start (name: ARRAY OF CHAR);

The first question is: Is the new part of the Dialog interface complete or need we some correction and extension ?

The reason why Ivan would like to insert those 2 procedures is: There exist a lot of procedures for this task and they are widely used.

I know the following old Open Commands
(1) InfoCmds.Start
(2) LibMisc.ShellExecute
(3) i21sysCalls.Start
(4) CpcCalls.Open
(5) HostTextConv.ShellExecute
The new Open Command is -> Dialog.Open

I know the following old Run Commands
(1) InfoCmds.Start2
(2) HostDialog.Start
The new Run Command is -> Dialog.Run

The second question is: Why don’t we use the old implementation of the equivalent procedure until we all agree with the new implementation?

- Helmut
User avatar
Robert
Posts: 1024
Joined: Sat Sep 28, 2013 11:04 am
Location: Edinburgh, Scotland

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Robert »

Zinn wrote:The first question is: Is the new part of the Dialog interface complete or need we some correction and extension ?
The second question is: Why don’t we use the old implementation of the equivalent procedure until we all agree with the new implementation?
Very sensible!

I didn't know about (or had forgotten about) HostDialog.Start.

I have just tried to use it - horrible! It flashed up a Dos window, then hid it before anyone could read it.
There are two sensible options: Don't open it, or let someone read it. I think it would be good to have the option, which means that the Start command should have a BOOLEAN input flag to control this option.

The proposed Docu for the proposed implementation is unclear: Does it open this window?, and can we read it before it disappears?


I agree that having these two routines in Dialog is a good idea - the discussion is about getting them right.
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Josef Templ »

There are more issues with the coding.

hwnd is an undefined variable in Open.

the waste of static variable memory has been doubled,it is now 16KB.

The stackSize is highly questionable:
It is 4KB but in Open it uses > 48KB for local variables.
In Run it uses > 16KB.
Seems to work by accident only.

Waiting for callNameTaken results in an endless loop if thread creation fails.

Dialog.IsWine() must not be called in a thread.
Look what can happens, e.g. if somebody inserts a Log output in the implementation
of the wineHook, this could lead to calling the garbage collector, which in this case would work
on the wrong stack. The system will crash. It does not give sense to argue with the probability of
such an event to occur. This must not be possible at all.

What is DuplicateHandle used for?

I doubt that ExitThread releases all thread resources.
The Windows docu says: "Terminating a thread does not necessarily remove the thread object from the operating system. A thread object is deleted when the last handle to the thread is closed."
Now the question is if there is such a handle and I think the answer is yes.
CreateThread returns a handle and that handle needs to be closed.

Many variables are essentially unused and can be removed.

My proposal for the improvements is issue-#104a.
Note the "a" at the end.

See the diffs under http://redmine.blackboxframework.org/pr ... 7e2ddc258a.

Please test under wine.

- Josef
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Josef Templ »

My proposal for the improvements is in issue-#104a.
Note the "a" at the end.

See the diffs under http://redmine.blackboxframework.org/pr ... 7e2ddc258a.

The voting must be interrupted.
It does not give sense to merge the current version into master
due to very poor code quality. In this case: speed kills.

I have posted a more detailed comment in the issue discussion.

- Josef
Ivan Denisov
Posts: 1700
Joined: Tue Sep 17, 2013 12:21 am
Location: Russia

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Ivan Denisov »

Please test under wine.
Works well.

Josef, I like how you improve the code. Thank you very much.
Now there will be no need to improve it in the beta version.

I do not see the reasons for suspend voting, so I will create voting soon.
User avatar
Josef Templ
Posts: 2047
Joined: Tue Sep 17, 2013 6:50 am

Re: issue-#104 calling URLs and commands (InfoCmds capabilit

Post by Josef Templ »

Ivan Denisov wrote:
Please test under wine.
Works well.

Josef, I like how you improve the code. Thank you very much.
Now there will be no need to improve it in the beta version.

I do not see the reasons for suspend voting, so I will create voting soon.

Thanks, Ivan.

The one thing that concerns me is backwards compatibility.
We have removed the procedure HostDialogs.Start, for good reasons, but so far it was
the only way of creating a link that opens an external app. There may be such links in
documents created by some user and they would not work any more.
Therefore I would suggest to add a trivial HostDialogs.Start as shown below.

Code: Select all

PROCEDURE Start*(name...); (* for backwards compatibility with BlackBox 1.6 *)
BEGIN Dialog.Run(name)
END Start;
- Josef
Post Reply