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

Already on GitHub? Sign in to your account

Adds canvas to color dict in rstdocument #882

Merged
merged 5 commits into from Jan 11, 2013

Conversation

Projects
None yet
3 participants
Contributor

shayneoneill commented Dec 31, 2012

This adds 'canvas' to the dict of colors in an RstDocument. As it stands canvas.before or canvas is not particularly useful for dealing with an RstDocument. This adds 'canvas' to the colors dictproperty allowing the background color to be updated

Owner

shayneoneill commented on d7ff355 Dec 31, 2012

Critique welcome. First time I've done a push request!

Owner

tshirtman commented Dec 31, 2012

I think i would call it background instead of canvas, as this one can be confusing on the role of this value.

Contributor

shayneoneill commented Jan 3, 2013

Ok. That seems sensible. I might give that a try. Not entirely sure the best way to update my pull request though :/

@shayneoneill shayneoneill commented on an outdated diff Jan 3, 2013

kivy/uix/rst.py
@@ -72,12 +72,15 @@
from kivy.animation import Animation
from kivy.logger import Logger
-from docutils.parsers import rst
-from docutils.parsers.rst import roles
-from docutils import nodes, frontend, utils
-from docutils.parsers.rst import Directive, directives
-from docutils.parsers.rst.roles import set_classes
-
+try:
@shayneoneill

shayneoneill Jan 3, 2013

Contributor

Let me know if I'm doing this part right. It seemed dropping out of the whole app with a warning about the import seemed the best approach to me, because I cant see how one could possibly recover from the error.. Maybe this is incorrect?

@shayneoneill shayneoneill commented on an outdated diff Jan 3, 2013

kivy/uix/rst.py
@@ -411,6 +415,7 @@ class RstDocument(ScrollView):
'''
colors = DictProperty({
+ 'background': 'ffffff',
@shayneoneill

shayneoneill Jan 3, 2013

Contributor

Is 'ffffff' an appropriate replacement for .9 .905 .910 ?

Owner

tito commented Jan 4, 2013

Well, i could merge, without the exception approach.

You could try/catch to display a simpler message, but re-raise the exception then. Otherwise, you will fail on the next usage of something from the docutils.

Contributor

shayneoneill commented Jan 4, 2013

I think though that not having docutils is a showstopper bug, in that the code will simply crash later on without much assistance as to why however, and since docutils isnt standard library the developer does need to know that its needed , if he's developing from raw sources.

Whats the prefered approach for dealing with this sort of dependency?

Owner

tito commented Jan 7, 2013

Just re-raise the exception. Don't hide the original exception :)

Contributor

shayneoneill commented Jan 8, 2013

Right but the original exception isn't really that user friendly, I figured actually explaining it (because its going to affect the majority of non technical users, since docutils is non-standard and not that common) might be a better approach?

Owner

tito commented Jan 8, 2013

You can explain, but not move the exception so something else. It explicit to have a ImportError docutils.XXX than AttributeError or KeyError later during execution. But i didn't tested :)

Contributor

shayneoneill commented Jan 8, 2013

Ah ok. Maybe I'm misunderstanding how the exception system works. Is there an example around somewhere I can look at for a prefered way of doing it?

Contributor

shayneoneill commented Jan 8, 2013

I'll freely admit python exceptions are something I've never actually bothered to read the manual for lol. Which is a bit lame considering how long I've been tinkering with python for haha

Contributor

shayneoneill commented Jan 10, 2013

Ok for now, lets drop it and worry about that later I guess.

@tito tito added a commit that referenced this pull request Jan 11, 2013

@tito tito Merge pull request #882 from shayneoneill/master
Adds canvas to color dict in rstdocument
bb1befb

@tito tito merged commit bb1befb into kivy:master Jan 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment