Add readOnly feature for Cantor's Worksheet
AbandonedPublic

Authored by sirgienko on Sep 4 2018, 7:04 PM.

Details

Summary

[T4760] Add readonly feature for Worksheet: if user open worksheet file and don't have backend for it, worksheet assigned as 'ReadOnly', that means, what command entries are become non-editable and non-executable and also some buttons, like 'Restart Worksheet', are hided.

It's finish realization T4760

Diff Detail

Repository
R55 Cantor
Branch
worksheet-add-readonly-feature
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2570
Build 2588: arc lint + arc unit
sirgienko created this revision.Sep 4 2018, 7:04 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptSep 4 2018, 7:04 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
sirgienko requested review of this revision.Sep 4 2018, 7:04 PM
sirgienko edited subscribers, added: asemke, Cantor, filipesaraiva; removed: kde-edu.

In this moment info message for this case (worksheet.cpp::1036) haven't updated, so I suggest to discuss the new info-message here.

asemke added a comment.Sep 4 2018, 8:00 PM

@sirgienko my proposal for the message would be:

i18n("% was not found. Editing is not possible", m_backendName);

and we should switch from KMessageBox::error() to KMessageBox::warning or KMessageBox::information.

Furthermore, we need to take care of the main menu. Many menu entries should be hidden for read-only worksheets. I think for non-editable worksheets we can show the "File" sub-menu completely and the "View" sub-menu with zooming items only and the "Settings" sub-menu. Everything else should be hidden for read-only worksheets and shown again when the user switches to another tab with an editable worksheet.

src/cantor_part.cpp
632

We should try to also show the proper icon in case the worksheet is read only. Furthermore, we can add "[read-only]" string to the caption to make it more clear and obvious that the currently active worksheet is read only.

src/commandentry.cpp
430

Is this really required if we don't allow any interaction for the item?

src/worksheet.cpp
1033–1034

use already here m_backendName directly?

1111

No syntax highlighting for a saved worksheet in the read only mode?

src/worksheet.h
88

should be defined const.

sirgienko added inline comments.Sep 4 2018, 8:36 PM
src/cantor_part.cpp
632

Ok

src/commandentry.cpp
430

This patch doesn't forbid completions in worksheet (for example, if we add completion in Latex, or Markdown entries (for latex and markdown langs, of cource), we could use it even in read only mode)

src/worksheet.cpp
1033–1034

Good idea.

1111

Highlighting, even from KSyntaxHighlighting, contains in plugin.
So, no plugin - no highlighting.
But well, it's not very good solution, I fix it on disable session highlighting only.

src/worksheet.h
88

Another functions, like isLoadingFromFile, don't defined as const.

sirgienko updated this revision to Diff 41054.Sep 5 2018, 2:33 PM

Update diff

sirgienko marked 4 inline comments as done.Sep 5 2018, 2:35 PM
sirgienko updated this revision to Diff 41056.Sep 5 2018, 2:59 PM

Update info message

filipesaraiva added inline comments.Sep 7 2018, 2:57 PM
src/worksheet.cpp
862

Is it working correctly? I tested with Python 3 backend but the line is null: <Worksheet backend="">.

sirgienko added inline comments.Sep 7 2018, 3:04 PM
src/worksheet.cpp
862

Good find, Filipe!
I forget, that for created session (which, obviciously not loaded) m_backendName will be Empty.

sirgienko updated this revision to Diff 41158.Sep 7 2018, 3:10 PM

Fix bug with m_backendName

sirgienko marked an inline comment as done.Sep 7 2018, 3:11 PM
filipesaraiva added a comment.EditedSep 7 2018, 9:11 PM

Thanks @sirgienko. It is an important feature to be added to Cantor.

Despite I liked the idea of a read-only mode only to command entries while the text entries can be modified, I don't know if we can call it a "real" read-only mode. I don't know if our users wait a behavior like it when they know Cantor has a read-only mode.

However I think we need to release it and wait some feedback from the users.

src/cantor_part.cpp
632

Change QLatin1String to i18n because this string will be translated to others languages, and put a blank space before the first [.

Despite I liked the idea of a read-only mode only to command entries while the text entries can be modified, I don't know if we can call it a "real" read-only mode. I don't know if of our users wait a behavior like it when they know Cantor has a read-only mode.

You have a good point: we could call it 'protected-mode', for example. Or another name, which will be more relevant to this behaver.

What about "limited mode" name? It's relevant to program behave: some functionality disable, so user have limited functionality of Cantor.

asemke added a comment.Sep 9 2018, 8:59 AM

What about "limited mode" name? It's relevant to program behave: some functionality disable, so user have limited functionality of Cantor.

@sirgienko @filipesaraiva Allowing to partially edit the worksheet will cause additional confusion on the user side, I think, and will also make additional implementation required on our side. I'd be more strict here and to use a real read-only mode for the whole worksheet where no changes are allowed at all if the backend is not installed. From the practical perspective this is also perfectly fine for the typical use cases. Such a worksheet is usually shared to other people who should

  • either work on this code and to be able to modify it - the backed is required in this case
  • or be only able to consume/read it - no backed required in this case

It is usually not like I share a maxima notebook with somebody and he/she wants to only edit the text comments to the code or to add an image. It is about the actual code in 99.99% of all cases. So, it's usually really an "either-or" decision.

Also, such a read-only mode would be similar to "Readers" provided by the commercial products where the user can only read the shared notebook/worksheet without no editing possible.

sirgienko added a comment.EditedSep 9 2018, 3:50 PM

What about "limited mode" name? It's relevant to program behave: some functionality disable, so user have limited functionality of Cantor.

@sirgienko @filipesaraiva Allowing to partially edit the worksheet will cause additional confusion on the user side, I think, and will also make additional implementation required on our side.

I disagree with it: I have tried to realise full readonly mode (prototype of this mode, of course) and it's require more code changes, than this partial read only mode.

I'd be more strict here and to use a real read-only mode for the whole worksheet where no changes are allowed at all if the backend is not installed. From the practical perspective this is also perfectly fine for the typical use cases. Such a worksheet is usually shared to other people who should

  • either work on this code and to be able to modify it - the backed is required in this case
  • or be only able to consume/read it - no backed required in this case

    It is usually not like I share a maxima notebook with somebody and he/she wants to only edit the text comments to the code or to add an image. It is about the actual code in 99.99% of all cases. So, it's usually really an "either-or" decision.

Yes (but maybe simpler to share worksheet in pdf format: it's even don't need Cantor), but in this solution, you can edit/not edit worksheet, if you want. And in full read only mode, you can't edit worksheet, even if you want. So, strict mode fits 99.99% cases, and "limited mode" fits 99.99% + 0.001% (when you need edit worksheet, without code changing, when you haven't backend).

P.S. PDF of my Octave test worksheet:

asemke added a comment.Sep 9 2018, 7:29 PM

I disagree with it: I have tried to realise full readonly mode (prototype of this mode, of course) and it's require more code changes, than this partial read only mode.

If it required more code changes than this was most probably not the most optimal solution. Did you try to re-implement the relevant events in WorksheetView and swallow the event in the read-only mode?

I'd be more strict here and to use a real read-only mode for the whole worksheet where no changes are allowed at all if the backend is not installed. From the practical perspective this is also perfectly fine for the typical use cases. Such a worksheet is usually shared to other people who should

  • either work on this code and to be able to modify it - the backed is required in this case
  • or be only able to consume/read it - no backed required in this case

    It is usually not like I share a maxima notebook with somebody and he/she wants to only edit the text comments to the code or to add an image. It is about the actual code in 99.99% of all cases. So, it's usually really an "either-or" decision.

Yes (but maybe simpler to share worksheet in pdf format: it's even don't need Cantor), but in this solution, you can edit/not edit worksheet, if you want. And in full read only mode, you can't edit worksheet, even if you want.

You _can_ edit the worksheet if you install the backend. Sharing pdf files is different since here there is no chance at all to edit the file. Again, it's all about the code in the most cases and not about text comments, etc. If you cannot edit the code and execute it, you won't work with the worksheet at all. You'll only want to consume it, i.g. to read it.

So, strict mode fits 99.99% cases, and "limited mode" fits 99.99% + 0.001% (when you need edit worksheet, without code changing, when you haven't backend).

Yes. But you'll need to explain to the user first what this "limited mode" is about and what is possible and what not in this mode. This is something that can cause confusions which I propose to completely avoid.

P.S. PDF of my Octave test worksheet:

Okay, @asemke , I will create another phabricator revison with full read only functionality and we could compare them.

Going back to this discussion.

I am discussing with some developers here in LaKademy. Maybe could be interesting just avoid the run of commands, but allow the edition to all kind of entries. This way, users can create/edit completely worksheets/notebooks without a backend.

Anyway, there is not a "best" solution to it now if we don't have feedback from the users. I recommend release the feature and wait for the reactions.

Going back to this discussion.

I am discussing with some developers here in LaKademy. Maybe could be interesting just avoid the run of commands, but allow the edition to all kind of entries. This way, users can create/edit completely worksheets/notebooks without a backend.

Anyway, there is not a "best" solution to it now if we don't have feedback from the users. I recommend release the feature and wait for the reactions.

To allow the user to edit already existing command entries can lead to confusing results in cases where the output is already available in the worksheet - you can overwrite the input which won't correspond anymore to the output that was saved before. I don't see any reason to allow this and to introduce this kind of confusions in the application.

Furthermore, Cantor is primarily a frontend for different CAS and scientific languages and not a text editor for latex or markdown or an image editor. In Cantor we create a maxima, octave, etc. worksheet with possibilities to add some text and image entries. It's not the other way around where we create a "text worksheet" with some possibilities to add maxima, octave, etc. command entries. So, a worksheet is always "backend centric" - this how we position Cantor. If the backend is not available, we make the worksheet read-only. Strictly read-only to make the implementation on our side easier and to also make the behavior in the application clear and consistent. If we show "worksheet is read-only" it is much easier for the user to grasp that nothing is possible in this document than to understand the logic like "we call the worksheet read-only now but this is only kind of read-only and you still can modify this and that".

Having said this, the second try for this implementation uploaded by Nikita in D15841 will provide a more consistent behavior in the application. Also, that patch is in a better shape now after couple if iterations since it also takes care of enabling/disabling menus and of couple of other things. Let's rather go for D15841.

sirgienko abandoned this revision.Nov 7 2018, 4:54 PM

D16333 was merged.