Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix harmless traceback of sugar-control-panel #802

Closed
wants to merge 1 commit into from
Closed

Fix harmless traceback of sugar-control-panel #802

wants to merge 1 commit into from

Conversation

rhl-bthr
Copy link

Fixes #793

Tested on: Ubuntu 16.04, Sugar 0.112

@quozl
Copy link
Contributor

quozl commented Jul 21, 2018

Thanks. Reviewed to d153654. I don't like the solution, because it does not explain the problem. A hack. Mysteries should not be left unsolved, because they often come back later. Why is from jarabe.view.buddymenu import BuddyMenu failing, yet the import of BuddyIcon succeeds? Is it because of a recursive import? BuddyMenu is unique in that it appears twice in the traceback.

* error was caused by a recursive import

Fixes #793

Tested-by: Rahul Bothra <[email protected]> # Ubuntu 16.04
@rhl-bthr
Copy link
Author

I don't like the solution, because it does not explain the problem.

Thanks for pointing out. I have added comments and updated the commit message

Is it because of a recursive import ?

Yes,

@quozl
Copy link
Contributor

quozl commented Jul 21, 2018

Is there an alternative solution? Net effect of a326c5f is to defer the import until the palette is created. At what elapsed time in the user experience? During shell startup before the activity ring is rendered, or when the icon is hovered over or clicked? We have some slow systems that run Sugar, and adding delay in the latter case would be unwelcome.

@rhl-bthr
Copy link
Author

I don't have access to any slower system, is there an alternative on how I can test for delay in slower systems on my current system

@quozl
Copy link
Contributor

quozl commented Aug 11, 2018

You can simulate a slower system by configuring a virtual machine hypervisor to restrict the CPU time available. You can then add logger.error calls to critical points in the shell startup. For the OLPC XO, the Python files are pre-compiled (to .pyo files), so you should run one startup test and discard the results before testing again.

However, you may find logger.error calls sufficiently accurate to figure out when your extra delay is incurred.

Remember also that sugar-control-panel is very rarely used, yet favoritesview.py and buddyicon.py are used heavily.

@rhl-bthr
Copy link
Author

While I am not getting time to test the delay, I can't see an alternative solution to this

Remember also that sugar-control-panel is very rarely used, yet favoritesview.py and buddyicon.py are used heavily.

I agree. Should we keep this then ?

@quozl
Copy link
Contributor

quozl commented Sep 17, 2018

Perhaps refactor to avoid the recursion?

Look at the traceback again, and consider each import from the top.

Look at extensions/cpsection/frame/model.py; it imports get_view from src/jarabe/frame/__init__.py only in order to reach Frame.settings in frame.py. Nothing else. If Frame.settings was in a separate source file instead of in frame.py, none of the other parts would be imported. The import recursion might be avoided.

What alerted me to this was the PyGIWarning for Gtk and the other libraries. sugar-control-panel doesn't have a GUI. Then no need to add require_version.

The reason there was only one Frame.settings to begin with is that each call to Gio.Settings for the same schema id creates a redundant object, consuming numerous resources, and not delivering signals. See a535877 for details.

@chimosky
Copy link
Member

chimosky commented Feb 6, 2019

@pro-panda what's the status of this, I hadn't been paying much attention until I recently tried using sugar-control-panel

@rhl-bthr
Copy link
Author

rhl-bthr commented Feb 8, 2019

@chimosky, thanks for bringing my attention to this. I'm not sure what did you mean by "status".

Stale. Closing

@rhl-bthr rhl-bthr closed this Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants