Opened 10 years ago

Closed 10 years ago

Last modified 6 years ago

#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)

sugar-toolkit.patch (1.0 KB) - added by alsroot 10 years ago.

Download all attachments as: .zip

Change History (22)

Changed 10 years ago by alsroot

comment:1 follow-up: Changed 10 years ago by 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.

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".

comment:2 follow-up: Changed 10 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 10 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: Changed 10 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 10 years ago by alsroot

Replying to 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 :)

guess, we should create restoration sub(underground?)group within Activity Team :D

comment:6 Changed 10 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: Changed 10 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: Changed 10 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: Changed 10 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: Changed 10 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 10 years ago by FGrose

  • Bug Status changed from Unconfimed to Needinfo

comment:12 Changed 10 years ago by FGrose

  • Cc eben added

comment:13 in reply to: ↑ 10 Changed 10 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 10 years ago by tomeu

  • Resolution set to fixed
  • Status changed from new to closed

comment:15 follow-up: Changed 10 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 10 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 10 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 10 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 10 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:20 Changed 6 years ago by godiard

  • Milestone 0.84 deleted

Milestone 0.84 deleted

comment:21 Changed 6 years ago by dnarvaez

  • Component changed from sugar-toolkit to Sugar
Note: See TracTickets for help on using tickets.