#428 closed defect (fixed)
connect('map') to activity widow not to current canvas
Reported by: | alsroot | Owned by: | marcopg |
---|---|---|---|
Priority: | Unspecified by Maintainer | Milestone: | |
Component: | Sugar | Version: | 0.83.x |
Severity: | Unspecified | Keywords: | r+ |
Cc: | eben | Distribution/OS: | Unspecified |
Bug Status: | Needinfo |
Description
in fact, set_canvas() is attractive function to use it in activity's logic(for example to switch tabs) and thats not obvious that read_file() will be invoked on every set_canvas() ('map' event will be triggered)
Attachments (1)
Change History (22)
Changed 14 years ago by alsroot
comment:1 follow-up: ↓ 3 Changed 14 years ago by wadeb
comment:2 follow-up: ↓ 4 Changed 14 years ago by wadeb
BTW, alsroot, checkout my Finance activity for a "screen" based approach to managing the canvas. I use it in Typing Turtle as well.
comment:3 in reply to: ↑ 1 Changed 14 years ago by alsroot
Replying to wadeb:
Be careful with this one - I realized early on that 'read_file' was called by 'set_canvas' and took care in my activities' startup code to respect that.
thats not me but infoslicer's author (me is just lazy to change activity's complicated code and prefer change it in sugar-toolkit level:)
comment:4 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 14 years ago by alsroot
Replying to wadeb:
BTW, alsroot, checkout my Finance activity for a "screen" based approach to managing the canvas. I use it in Typing Turtle as well.
will do, after polishing infoslicer a bit :)
comment:5 in reply to: ↑ 4 Changed 14 years ago by alsroot
comment:6 Changed 14 years ago by wadeb
Heh, it's just you so far :) But you're about as productive as a whole group.
comment:7 follow-up: ↓ 8 Changed 14 years ago by tomeu
If I remember correctly, read_file is called in the canvas map handler because Write could set the file to load before abiwidget was mapped.
This patch would break it because the main window is mapped before its children, though maybe abiwidget has been fixed since.
Anyway, I think that the idea is to call read_file after the activity has been set up, but before anything has drawn on the screen. Other suggestions?
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 14 years ago by alsroot
Replying to tomeu:
If I remember correctly, read_file is called in the canvas map handler because Write could set the file to load before abiwidget was mapped.
This patch would break it because the main window is mapped before its children, though maybe abiwidget has been fixed since.
I tried start_new/load_jobject with git Write and it works w/o any critical difference
Anyway, I think that the idea is to call read_file after the activity has been set up, but before anything has drawn on the screen. Other suggestions?
but attached patch doesn't break this idea
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 14 years ago by tomeu
Replying to alsroot:
Replying to tomeu:
If I remember correctly, read_file is called in the canvas map handler because Write could set the file to load before abiwidget was mapped.
This patch would break it because the main window is mapped before its children, though maybe abiwidget has been fixed since.
I tried start_new/load_jobject with git Write and it works w/o any critical difference
That's nice, but I'm a bit worried about other activities. Checking all of them can be quite a bit of work.
What if in set_canvas we disconnect the previous handler if there's one and then connect to the new one?
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 13 Changed 14 years ago by alsroot
Replying to tomeu:
That's nice, but I'm a bit worried about other activities. Checking all of them can be quite a bit of work.
from soas-1's only 3 activity uses map event for set_cavas's widget and they are ok (in fact they uses that in #267 way)
What if in set_canvas we disconnect the previous handler if there's one and then connect to the new one?
problem still exists - if user wants set_canvas() to set current tab(after changing tab in ActivitToolbox) it will catch one read_file() per each tab changing
comment:11 Changed 14 years ago by FGrose
- Bug Status changed from Unconfimed to Needinfo
comment:12 Changed 14 years ago by FGrose
- Cc eben added
comment:13 in reply to: ↑ 10 Changed 14 years ago by tomeu
- Keywords r+ added; r? removed
Replying to alsroot:
Replying to tomeu:
That's nice, but I'm a bit worried about other activities. Checking all of them can be quite a bit of work.
from soas-1's only 3 activity uses map event for set_cavas's widget and they are ok (in fact they uses that in #267 way)
What if in set_canvas we disconnect the previous handler if there's one and then connect to the new one?
problem still exists - if user wants set_canvas() to set current tab(after changing tab in ActivitToolbox) it will catch one read_file() per each tab changing
Ok, if the only activity known as needing this was Write and this isn't needed there any more, then I think we should apply your patch.
comment:14 Changed 14 years ago by tomeu
- Resolution set to fixed
- Status changed from new to closed
comment:15 follow-up: ↓ 16 Changed 14 years ago by wadeb
- Resolution fixed deleted
- Status changed from closed to reopened
Ok, I have something to add to this ticket.
What if an activity needs to know *before starting up* whether it is a new instance or a resumed instance?
Currently, all of my activities set up their new instance state and then expect it to be clobbered by 'read_file' if this is a resume. But there are cases where that's not possible or has major issues.
For example, in a Terminal branch this weekend I added saving / restoring of the terminal state. This requires me to launch the child process differently depending on whether it's resumed or not. So, I *have* to know when the activity starts up whether I'm saving or resuming.
I think we should add a 'new_file' callback which is called when 'read_file' *will not* be called.
Also, I think the _file APIs are kind of poorly named. Wouldn't it be better to have resume_instance, save_instance and new_instance? If it's agreed, we could add these and then have the default implementations call read_file, write_file and nothing, respectively.
Regards,
Wade
comment:16 in reply to: ↑ 15 Changed 14 years ago by alsroot
Also, I think the _file APIs are kind of poorly named. Wouldn't it be better to have resume_instance, save_instance and new_instance? If it's agreed, we could add these and then have the default implementations call read_file, write_file and nothing, respectively.
in that case we will close #267 -- user could place his after_instance() invoking to new_instance()/resume_instance() to catch all possible threads of creating process
comment:17 Changed 14 years ago by garycmartin
Not sure if this helps, but some time back (after I asked for a similar callback) tomeu suggested I should just be checking handle.object_id, rather than the dubious trick I currently employ. Of course I've not actually implemented it for real but at least line 75 of Moon shows willing ;-)
http://git.sugarlabs.org/projects/moon/repos/mainline/blobs/master/moon.py
comment:18 Changed 14 years ago by alsroot
possible implementation of {new|resume|save|share}_instance scheme:
http://git.sugarlabs.org/projects/cartoon-builder/repos/mainline/blobs/master/shared.py
comment:19 Changed 14 years ago by wadeb
- Resolution set to fixed
- Status changed from reopened to closed
I created a new ticket #568 to track the API recommendations.
comment:21 Changed 10 years ago by dnarvaez
- Component changed from sugar-toolkit to Sugar
Be careful with this one - I realized early on that 'read_file' was called by 'set_canvas' and took care in my activities' startup code to respect that.
IE, don't initialize the default state *after* calling set_canvas.
That said, I *think* that everything will work fine if read_file is called after the activity's init returns.
And I agree that this change makes sense, it was always a "gotcha".