Improve octave login
ClosedPublic

Authored by sirgienko on Apr 27 2018, 7:56 PM.

Details

Summary

Before, when we work with octave and use 'Restart Backend' button, we suddenly get foreign output. So this commit fix it by adding backend cleanup of inner state in logout.

Test Plan
  1. Run octave backend without patch
  2. Write some entries and run they
  3. Reboot octave backend by the button and check, that the output changes
  4. Apply patch
  5. Check, that now all fine.

Diff Detail

Repository
R55 Cantor
Branch
improve-octave-login
Lint
No Linters Available
Unit
No Unit Test Coverage
sirgienko created this revision.Apr 27 2018, 7:56 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptApr 27 2018, 7:56 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript
sirgienko requested review of this revision.Apr 27 2018, 7:56 PM

Cleaning up in logout() is ok. Sending emit loginDone() in readOutput() is not very clean. Everything login related should be done in login(). What do you mean with "we emit this signal before we real finish preparation"? Is m_process->waitForReadyRead(); in OctaveSession::login() not enough to collect and to parse the initial output of octave?

Cleaning up in logout() is ok. Sending emit loginDone() in readOutput() is not very clean. Everything login related should be done in login(). What do you mean with "we emit this signal before we real finish preparation"? Is m_process->waitForReadyRead(); in OctaveSession::login() not enough to collect and to parse the initial output of octave?

Acording to Qt documentation

waitForReadyRead(): Blocks until new data is available for reading and the readyRead() signal has been emitted

And we have connect ( m_process, SIGNAL ( readyReadStandardOutput() ), SLOT ( readOutput() ) );
But we wait only for readyRead() signal and qt don't garantee, that the slot, connected to this signal, already have be executed.
So, we could emit loginDone before backend set m_prompt and m_tempDir. Now it doesn't happends, but it could be possible or I am wrong with it and it's not a problem.

I was thinking about getting the octave prompt like lua backend: connect output to special function for reading prompt, which at the end of work reconnect output to parsing functions and emit loginDone signal.
Well, now lua emit loginDone signal in login function, so sometimes I loose first comand in the lua tests, because the backend run expression before reading the prompt and I got something like this:
reading the intro message "Lua 5.2.4 Copyright (C) 1994-2015 Lua.org, PUC-Rio\n> print(2+2)\n4\n> "

But maybe I have made a mistake, when have move the signal emit statement in this patch.

Cleaning up in logout() is ok. Sending emit loginDone() in readOutput() is not very clean. Everything login related should be done in login(). What do you mean with "we emit this signal before we real finish preparation"? Is m_process->waitForReadyRead(); in OctaveSession::login() not enough to collect and to parse the initial output of octave?

Acording to Qt documentation

waitForReadyRead(): Blocks until new data is available for reading and the readyRead() signal has been emitted

And we have connect ( m_process, SIGNAL ( readyReadStandardOutput() ), SLOT ( readOutput() ) );
But we wait only for readyRead() signal and qt don't garantee, that the slot, connected to this signal, already have be executed.
So, we could emit loginDone before backend set m_prompt and m_tempDir. Now it doesn't happends, but it could be possible or I am wrong with it and it's not a problem.

I was thinking about getting the octave prompt like lua backend: connect output to special function for reading prompt, which at the end of work reconnect output to parsing functions and emit loginDone signal.
Well, now lua emit loginDone signal in login function, so sometimes I loose first comand in the lua tests, because the backend run expression before reading the prompt and I got something like this:
reading the intro message "Lua 5.2.4 Copyright (C) 1994-2015 Lua.org, PUC-Rio\n> print(2+2)\n4\n> "

But maybe I have made a mistake, when have move the signal emit statement in this patch.

Yes, this all is a bit inconsistent in Cantor for different backends. I'd rather go for a solution where all the initial stuff is done in login() only. So, parsing of the promt, if required, emitting of signals etc. should be done in login(). This is the case for example in juliasession.cpp where the output is read until "ready" comes. parseOutput() should be used for parsing of the expression outputs only. This would lead to a more clear and consistent architecture in Cantor. Once this is done in octavesession, this should also solve the problem you propose to fix here.

Yes, this all is a bit inconsistent in Cantor for different backends. I'd rather go for a solution where all the initial stuff is done in login() only. So, parsing of the promt, if required, emitting of signals etc. should be done in login(). This is the case for example in juliasession.cpp where the output is read until "ready" comes. parseOutput() should be used for parsing of the expression outputs only. This would lead to a more clear and consistent architecture in Cantor. Once this is done in octavesession, this should also solve the problem you propose to fix here.

Well, I think I have understood you now, so I remove the emit changes and will start to work on solution, which have all init logic in login and don't return, until all preparation will be done.

sirgienko updated this revision to Diff 33266.Apr 28 2018, 8:34 PM

Update revision.
Also fix small misspelling in logout.

asemke accepted this revision.Apr 30 2018, 7:41 AM
This revision is now accepted and ready to land.Apr 30 2018, 7:41 AM
sirgienko edited the summary of this revision. (Show Details)Apr 30 2018, 3:19 PM
sirgienko closed this revision.Apr 30 2018, 3:19 PM