Save As Popup #327
Save As Popup #327
Conversation
|
Allright I just tested this feature.
Some serious thought needs to go into this interaction, as it is a very important one. We must take into consideration that Sugar already saves the object if the user changed activities or views before, even if it was the first instance (of object creation). Perhaps the alert could work this way: It could have default actions for each question. Pressing Stop button again will simply take default actions (I instinctively tried this, i.e. telling the system I really want to stop, don't ask me questions). Take this as an idea:
|
|
I agree with @icarito about the issues with the current implementation. Here are a few of my takes on it:
|
|
@icarito @samdroid-apps I have fixed some of the issues. Please test the newest commit. @samdroid-apps When the user selects the |
I thought that one of the main reasons to add this change was the delete journal clutter by letting the user delete entries more quickly. What is the use case of saving the metadata but not the entry data? |
|
@samdroid-apps Mr. Anderson asked me to build it that way. |
|
"When the user selects the discard option, the activity data (for eg. text in case of Write activity) doesn't get saved on close." But it's already saved before close! You mean it gets deleted? |
|
icarito Yes. When the activity is open, a duplicate copy is created. If the user selects to 'discard' the data, the duplicate copy is deleted without saving the data. And if the user selects to save the data, the duplicate copy is retained by the new name provided by the user in the text entry and the data is saved to that dupicate instance without overwriting the original instance. |
|
Here's comments by Tony on private thread: You open Paint and create a red X and save it as x. Then you open x and change the image to a blue O. You want to save it as O, so you change the name. Suppose you open Paint to show someone how to select a brush and give it a color. You scribble something. When asked for a name, you click on 'quit' because there is not need to save this image. The alert should be simple: Please give your project a title. [Untitled] Save Quit The default title is 'Untitled' not 'Write.activity'. If the user clicks save with the title unchanged, it is ignored. If the user clicks on Quit, the activity quits (with no save of the Document - but the metadata is saved). if the user resumes an activity the alert shows: Change the title to save as a new document [X] Save Quit The user changes the title [X] to [Y]. Then Save saves Y to the Journal and X is still in the Journal as well. If the user doesn't change and clicks Save, the X object is deleted and the new X version is saved. if the user clicks on Quit - the document is not saved, the original X document is in the Journal I can't find Tony's github user to mantion him. My comment is: "Change the title to save as a new document": seems long to me. Also it's not always a document (e.g. Browse session). if the user clicks on Quit - the document is not saved, the original X document is in the Journal |
|
"If the user selects to 'discard' the data, the duplicate copy is deleted without saving the data. " This is the part that I don't understand, Sugar will save the data on every view change. So you mean it will delete the duplicate (that may have been previously already saved). |
|
I tried the new version and it works much better. I'm not convinced by the text in the alert. You seem to imply two objects can't have the same name, but they can. Objects are identified by a unique hash id in the journal. Save as / Quit is a strange button label. One of Sugar's design principles is simplicity. |
src/sugar3/activity/activity.py
Outdated
|
|
||
| return True |
There was a problem hiding this comment.
After your patch I can't close ImageViewer or Chat. This is likely why.
There was a problem hiding this comment.
The reason 'ImageViewer' and 'Chat' didn't close is because these two activities override the 'can_close()' funtion of 'activitly.py' which is responsible to display my alert.
I have figured out a solution and commit a new tweak for the same.
|
Currently my main concern from a UI pov is that choosing "Stop" and then "Quit" will result in the user discarding his work. This is not intuitive to me. |
|
"Mr. Anderson asked me to build it that way." - That's a bad reason, you should believe in what you do! There are better ways to collect metadata than to clutter the journal. This has been a bad use of it. |
|
@icarito I agree. But the 'SaveAs' feature was proposed by Mr. Anderson and I'll have to abide by the requirements he specifies. |
|
This thread would be a lot more useful if you took the time to understand Sugar as well as this PR. "Mr. Anderson asked me to build it that way." - That's a bad reason, you should believe in what you do! There are better ways to collect metadata than to clutter the journal. This has been a bad use of it. This is crazy. I am mentoring this feature. Granted Utkarsh can proceed as he feels best but we don't need to involve him in these quibbles. Any Journal object consists of two elements: a metadata file and an (optional) data file. The data file normally conforms to a meta_type and is from the point of view of the user, a document. The metadata needs to be saved in every case. The document only if the user chooses. Note: that any user is free to delete any Journal object at any time without getting permission from anyone. |
|
Currently my main concern from a UI pov is that choosing "Stop" and then "Quit" will result in the user discarding his work. This is not intuitive to me. This is why you should discuss the issue. The alert offers two options: save and quit. The alert appears because the user has clicked on 'stop' to request terminating the activity. The current activity.py does indeed save the document without action by the user (we used to have 'keep' which gave the user the option to save and then continue. This was removed. However, the implementation is flawed. The jobject used by activity.py in case of resume is the original jobject_id. This means, unknown to the user, he is making irretrievable changes to his document. Hence, Utkarsh makes a copy of the original jobject and this is what is changed. This enables the save option after a name change, to keep both the original document and the new instance as a user should expect. |
|
When choosing to overwrite do you delete the previous copy or do you always keep a copy? If you take the trouble to read the description of the feature, you will know that if the user resumes an activity with a title and chooses not to change the title, then the modified document is saved and the original is deleted. This is exactly what happens now. If the user changes the name, then the new document is saved under its new name and the unmodified original is also in the Journal. |
|
If you have cosmetic changes make them on another PR. It makes it hard to see what you changed Who decides what changes are important and which are 'cosmetic'. This is the PR. |
|
The reason 'ImageViewer' and 'Chat' didn't close is because these two activities override the 'can_close()' funtion of 'activitly.py' which is responsible to display my alert. I have figured out a solution and commit a new tweak for the same. Utkarsh: 'can_close' is a function which can be supplied by the activity. In that case, the copy in activity.py is not called. What you need to do is post the alert after can_close. This way from the user's view, the alert will be seen before close. When the user responds to the alert, the activity can close. |
|
I tried the new version and it works much better. I'm not convinced by the text in the alert. You seem to imply two objects can't have the same name, but they can. Objects are identified by a unique hash id in the journal. From the user perspective, documents should have a unique name. Granted in standard software, different files may have the same name in different directories. We only have the Journal. Save as / Quit is a strange button label. The button options should be 'save' and 'quit'. The text on the button may differ from the start new case when the title is 'Untitled' or 'Write.activity' - perhaps: Please give a name to your project. In the resume case, the text should refer to the option to save (no change) or save as (change). Perhaps, Change the name of your project to save a new document. One of Sugar's design principles is simplicity. |
|
As promised here's a screenshot of Sugar 0.92's "save as" dialog: As per our meeting today I think Martin Dengler's proposal to land the user in the Journal after quitting with focus on Object Title would be a good solution. |
|
Correction screenshot is from Sugar 0.90.2 |
|
As you might expect, I totally disagree. First, by that time the damage has been done. Either the 'save as' This feature has two objectives: one or to simply save the modified document. This proposal addresses neither objective. You are not going to change your point of view so the only solution I Re: the screenshot. Help me - as I remember this dialog was modal and Tony On 07/15/2016 04:36 AM, Sebastian Silva wrote:
|
|
I think user testing can resolve such ux disagreements.
A setting to choose from both is not a wise design decision because there
is still a question of which is default and how easy it is to set.
Since Tony is advocating for a change, perhaps you could do a user test.
Have you done one before? :)
|
I think that user testing is invaluable there. But I think that @icarito very clearly addresses item number 1 - the proposal literally puts the user in front of a screen dedicated to managing activity metadata. For addressing proposal 2, did you want to make sugarlabs/sugar#699 false by default? I would support making that false by default, now that I understand the rational. But seriously, if you do some user testing and write up some scenarios for the users, I can also find some other people and carry out the same tests too! More testing the better! |
|
Sam I don't understand you. #327 satisfies both objectives. "The proposal literally puts the user in front of a screen dedicated to #699 is a completely different feature. Yes, it should not change Sugar I have no idea how to test the first item you describe - it has nothing User tests on #327 seem unnecessary, We all use it everyday on Linux, Tony On 07/15/2016 11:54 PM, Sam wrote:
|
|
@quozl I'll update sugarlabs/sugar-toolkit#4 accordingly after this PR is completed |
No, that's wrong, and may create a race condition. It may work most of the time, but sometimes it will fail, if the graphics device is momentarily delayed, and especially on multicore systems where the GTK+ program may move to idle processing before another core has finished graphics device update. It is better to connect to the
|
|
Also in 97c7836 when
For your testing, among the activities that implement I'm having trouble following the new logic path in |
|
Thanks for this testing. You are showing some serious ambiguity in the
core implementation of activity.py
A return of True from can_close should end processing except write_file.
Aside from user 'ctrl' + q or quit, the activity could be closed as a
result of the end of the Sugar session by shutdown or logout or by
shutdown of the system because of lower battery power.
As I recall, activity developers should assume the object changes made
in write_file will be saved.
For the quiz activity, I have had to set a flag in can_close because the
information needed by write_file is not available until the quiz
responses are complete.
This no-ops the write_file calls until after can_close has been called.
Tony
…On 05/02/2017 09:27 AM, James Cameron wrote:
Also in 97c7836
<97c7836>
when |save-as| is enabled,
* there is no busy cursor during close; this is a regression, which
you can fix by making the cursor busy again when the alert button
'Save' is pressed,
* the |_closing| signal is emitted early, which causes a
|save_title| in widgets.py to run, but is that necessary?
* the activity |can_close| is called only once, yet the learner can
continue to interact with the activity (which I've just tested
again) and may invalidate |can_close|; this is a regression on the
meaning of |can_close|, which you can fix by either making the
alert modal (which would be unfortunate), or calling |can_close|
again sometime later.
For your testing, among the activities that implement |can_close| are
Browse, Chat, Develop, FractionBounce, GetBooks, ImageViewer, Jukebox,
Maze, Physics, Read, and TurtleBlocks.
I'm having trouble following the new logic path in |close|; it seem
convoluted to call |_alert_confirmation| only for that method to find
out that the title has been changed, then set a flag |_pre_naming|
which is tested again later in |close|.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#327 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAULkunUw5MCo_otLoA5iNCjZpPb_1Dtks5r1oZvgaJpZM4JEg63>.
|
Fixed
_closing signal is now emitted only when the user takes an actions (Save/Don't save) from alert
After checking I found out that the 'can_close()' is called
_alert_confirmation code moved inside 'close()' to remove ambiguity |
|
@iamutkarshtiwari, thanks. I've tested all day and found many problems. It was easier to fix the problems and rework the code than go into detail explaining what I've fixed. I've pushed two branches:
Please consider;
My own TODO list;
|
- Gtk.EventBox is not needed, - show_all is used, so show is not needed, - documented some accesses to private members of an instance,
- move Gio.Settings access to sugar3.profile.get_save_as, - remove several instance members; flags, and the alert, - remove key-press-event and use accelerators instead, - use reference counted busy cursor implementation from home view, - register stop buttons and make them insensitive during stop alert, - reword the stop alert, add cancel button.
|
Thanks for taking my changes, you've encouraged me to look further at the problems. I've pushed 11 patches to my branch, please review and cherry-pick them; Progress was;
Also tested with shared activities; in particular Maze and Chat. |
- add tooltips to the buttons, for mouse-over explanations in context, - change the button labels and tooltip according to the context; resumed or new activity, unchanged or changed name.
- handle.object_id was being tested twice, so move conditional code into the first test. - tested with local activities only.
- it is the original title and does not always refer to the clone.
- joining a shared instance will change the title from the default, which would force every participant to undergo the stop alert, - so set the title again after it has finished changing.
- an Alert may now have a Gtk.Entry between the title and the buttons.
- use the original title instead
- but in this case, use the current jobject title
- move the copy to after the nested event loop so that metadata updates resulting from the start of sharing are not lost, - use datastore.copy to copy all metadata rather than initialise a new journal object, thus simplifying code and preserving statistics, - rename _jobject_clone to _jobject_old as it isn't really the clone, but rather the old object before it was cloned. Tested with shared activities; Maze and Chat.
iamutkarshtiwari
force-pushed
the
feature-saveAsPopup
branch
from
5eb9ef6
to
46336aa
Compare
|
@iamutkarshtiwari, thanks for taking my changes; have you reproduced the problem with focus on the Write activity with these patches applied? Any ideas on what causes it? I've not seen any other activity affected. I'm inclined to let it pass. |
|
Thanks for your effort and patience in making this possible. I think you
helped this feature become more useful and robust.
Tony
…On 06/01/2017 07:44 AM, James Cameron wrote:
Rebased and merged as 926a262
<926a262>,
83697d2
<83697d2>,
286387b
<286387b>,
b1d1129
<b1d1129>,
6bcd664
<6bcd664>,
3a574ae
<3a574ae>,
and ba6993d
<ba6993d>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#327 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAULkhVNIu1BZ0XDT7-Jb-TblSblTIDYks5r_kGjgaJpZM4JEg63>.
|
'Save As' popup added which appears on activity close. It allows user to provide instance name to the user. - https://www.youtube.com/watch?v=xcvBH7zzFBo
