search

straight55b / app-engine-patch

Automatically exported from code.google.com/p/app-engine-patch
0 stars 0 forks source link

mediautils: Automatic path adjustment in CSS files #178

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Put a complete CSS framework into a media folder, e.g. YAML 
(http://www.yaml.de/)
2. Generate the media files with manage.py generatemedia

What is the expected output? What do you see instead?
Currently mediautils does not touch the url() paths inside the CSS files, 
but it would be great if it could change relative paths (e.g. images/) to 
the corresponding path (e.g. 
$MEDIA_URL/$MY_APP/$PATH_FROM_MYAPP_TO_CSS_FILE/images/) automatically.

Example:
File: common/yamlcss/media/yaml/navigation/nav_shinybuttons.css
Image URL before: images/shiny_buttons/background.png
Image URL after: 
media/1/yamlcss/yaml/navigation/images/shiny_buttons/background.png

What version of the product are you using? On what operating system?
Latest stable release: 1.0.2.1 on Jun 18, 2009

Please provide any additional information below.
This would make plug&play for new CSS framework versions a lot easier.

Original issue reported on code.google.com by mbac...@gmail.com on 6 Jul 2009 at 4:41

GoogleCodeExporter commented 9 years ago 
My solution:

Add the following import at the top:
import re

Search for the following line:
cache[path] = cache[path].replace('$MEDIA_URL/', settings.MEDIA_URL)

Add the following replace code after that line:
cache[path] = re.sub(r'url\(["\']?(?<!/)([^:]*?)["\']?\)', 'url("%s%s/\\1")' % 
(settings.MEDIA_URL, os.path.dirname(handler % dict(kwargs))), cache[path])

Original comment by mbac...@gmail.com on 7 Jul 2009 at 7:24

GoogleCodeExporter commented 9 years ago 
Your code doesn't take into account that "handler" might be a function. Also, 
you'll 
need to support paths starting with "../", so you can access media from other 
apps 
(IOW, the replacement string should rather be a function that does some more 
complex 
processing of the path). Could you please provide a fix for that?

Original comment by wkornew...@gmail.com on 8 Jul 2009 at 8:45

GoogleCodeExporter commented 9 years ago 
I don't think that the function as handler thing is any problem here. The 
cache[path].replace thing does not take this into account, too.

Paths like ../ and ./ will work, too. Example:
File: common/yamlcss/media/yaml/navigation/nav_shinybuttons.css
Image URL before: ../myotherapp/images/shiny_buttons/background.png
Image URL after: 
media/1/yamlcss/../myotherapp/yaml/navigation/images/shiny_buttons/background.pn
g

Sure, it looks a little bit ugly, but it does work. Browsers are able to 
resolve the 
target URL anyway.

Original comment by mbac...@gmail.com on 8 Jul 2009 at 9:54

GoogleCodeExporter commented 9 years ago 
In my code I don't use the handler directly. I converted it into path at the 
top of the 
function, so it's safe. In your code you use handler directly, so it's not safe.

Regarding relative paths, I hope that really all browsers can handle it. I'd 
rather 
have path conversion, so we're on the safe, non-ugly side.

Original comment by wkornew...@gmail.com on 8 Jul 2009 at 10:07

GoogleCodeExporter commented 9 years ago 
Now I see what you mean. Sorry, do you think a isinstance(handler, str) would 
be enough 
to make it work?

I am not the uber-RegEx coder, maybe someone else can create a RegEx which 
solves the 
../ thing directly?

Original comment by mbac...@gmail.com on 8 Jul 2009 at 1:38

GoogleCodeExporter commented 9 years ago 
1) Probably isinstance(handler, basestring) (instead of str, so unicode is 
handled, 
too) is enough. Though, the whole conversion code should be moved into a 
separate 
function, so callable handlers can reuse that feature.

2) This can't be done with a simple regex, anyway. You can pass a function 
instead of 
the replacement string and that function will get the match object and return 
the 
replacement string. There, you can process the path and handle "../" correctly. 
I don't 
really have time to implement this for the next release.

Original comment by wkornew...@gmail.com on 8 Jul 2009 at 1:53

GoogleCodeExporter commented 9 years ago 
1) I have moved the CSS content things into a seperate function.

2) It can be done, I have found a solution. Didn't want to create an inner 
function, 
because it would require some variables.

I have attached a unified patch/diff to this post, it also fixes two other 
issues:
The files with target combined-ltr.css and combined-%(LANGUAGE_DIR)s.css are 
now 
combind together into combined-ltr.css. Before my change only combined-
%(LANGUAGE_DIR)s.css was generated and combined-ltr.css was skipped, because 
the file 
already existed.

I also removed the sorted() from the media keys loop, because this destroys the 
users 
application setup order.

It works very well for me and I can finally develop locally, because of the 
change 
mentioned above.

My next idea would be to zip the generated media folder and serve it with 
zipserve. 
That would reduce the amount of files a lot, because I am currently very near 
to the 
1000 files limit.

Please consider the changes for the next release.

Best regards

Original comment by mbac...@gmail.com on 9 Jul 2009 at 10:27

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for the patch! Works great! I've committed most of your patch. But I've 
added 
a closure because that's more correct. The inner function (closure) has access 
to all 
variables of the outer function (if that was your concern).

Regarding sorted(): Dicts are in arbitrary order, so there really is nothing to 
"destroy" because your setup order is ignored, anyway (even without sorted()). 
Also, 
why do you use combined-ltr.css? The documentation says it must contain 
%(LANGUAGE_DIR)s. I haven't added those patches because IMHO that's a 
workaround for 
something you should rather change in your code.

Original comment by wkornew...@gmail.com on 9 Jul 2009 at 1:14

GoogleCodeExporter commented 9 years ago 
Please add the changes related to the target file, otherwise I can't use YAML 
with 
mediautils, please see:

settings.add_app_media('combined-ltr.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
)
settings.add_app_media('combined-rtl.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
    'yamlcss/yaml/add-ons/rtl-support/core/base-rtl.css',
)

You see, the rtl-support file is in a different location and has a very 
different 
name. %(LANGUAGE_DIR)s does not help here. As I said, I want to do plug&play 
with CSS 
frameworks and new releases.

You are right about the sorted() thing, thought.

Original comment by mbac...@gmail.com on 9 Jul 2009 at 6:15

GoogleCodeExporter commented 9 years ago 
I have to add:

The example above works, as long as it is stand-alone. But as soon as there is 
media 
for 'combined-%(LANGUAGE_DIR)s.css', the files above would not be added anymore.

If you add the code I gave you, this can be reduced to:

settings.add_app_media('combined-%(LANGUAGE_DIR)s.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
)
settings.add_app_media('combined-rtl.css',
    'yamlcss/yaml/add-ons/rtl-support/core/base-rtl.css',
)

Original comment by mbac...@gmail.com on 9 Jul 2009 at 6:23

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
I'd solve that problem this way:
settings.add_app_media('combined-%(LANGUAGE_DIR)s.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
    'yamlcss/yaml/add-ons/rtl-support/core/base-%(LANGUAGE_DIR)s.css',
)

And I'd add an empty base-ltr.css in yamlcss/yaml/add-ons/rtl-support/core/

Original comment by wkornew...@gmail.com on 9 Jul 2009 at 8:13

GoogleCodeExporter commented 9 years ago 
Regarding your changes, I haven't added them because they won't work. Doesn't 
base-
rtl.css assume that it comes after the other CSS files? There is absolutely no 
guarantee that it will come last. I'll try to find some other solution. One 
possible 
solution is to add a handler function:

settings.add_app_media('combined-%(LANGUAGE_DIR)s.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
    empty_fallback('yamlcss/yaml/add-ons/rtl-support/core/base-
%(LANGUAGE_DIR)s.css'),
)

That function would try to read the given file and if it doesn't exist it 
simply 
returns an empty string. The function would look like this:

import os
def empty_fallback(path):
    def handler(**kwargs):
        from mediautils.generatemedia import get_file_content
        try:
            return get_file_content(path, cache={}, **kwargs)
        except IOError:
            return ''
    # Currently, handler names may not contain slashes
    handler.name = path.replace('/', '--')
    return handler

Original comment by wkornew...@gmail.com on 9 Jul 2009 at 10:02

GoogleCodeExporter commented 9 years ago 
The other solution would be my first code snippet, but as I said, that does 
only work 
as long as it is stand-alone:

settings.add_app_media('combined-ltr.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
)
settings.add_app_media('combined-rtl.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
    'yamlcss/yaml/add-ons/rtl-support/core/base-rtl.css',
)

This would guarantee the correct order.

I still think that those %()s things shouldn't be required at all. IMHO it's a 
cool 
feature to have and use, but you should be able to specify the targets directly.

Original comment by mbac...@gmail.com on 10 Jul 2009 at 7:53

GoogleCodeExporter commented 9 years ago 
Another idea, to keep the order from the beginning, would be to change 
add_app_media so 
that it directly resolves the correct target, so that all files directly belong 
to the 
correct target inside the COMBINE_MEDIA structure.

Original comment by mbac...@gmail.com on 10 Jul 2009 at 7:59

GoogleCodeExporter commented 9 years ago 
add_app_media can't do it because the media generator needs additional 
information

What's bad about the handler concept? Maybe a nicer handler would be like this

settings.add_app_media('combined-%(LANGUAGE_DIR)s.css',
    'yamlcss/yaml/core/base.css',
    'yamlcss/yaml/screen/forms.css',
    'yamlcss/yaml/add-ons/microformats/microformats.css',
    for_taget('combined-rtl.css',
        'yamlcss/yaml/add-ons/rtl-support/core/base-rtl.css'),
)

I might be able to implement this for the next release.

Original comment by wkornew...@gmail.com on 10 Jul 2009 at 9:44

GoogleCodeExporter commented 9 years ago 
Great idea, thanks in advance! :)

Original comment by mbac...@gmail.com on 11 Jul 2009 at 9:59

GoogleCodeExporter commented 9 years ago 
Any ETA yet?

Original comment by mbac...@gmail.com on 22 Jul 2009 at 9:11

GoogleCodeExporter commented 9 years ago 
I'm very busy with other projects and I don't have any time for 
app-engine-patch right 
now. Don't expect anything anytime soon.

Original comment by wkornew...@gmail.com on 23 Jul 2009 at 4:13

GoogleCodeExporter commented 9 years ago 
Here is another workaround, this diff makes the get_file_content method return 
an empty 
string if the file does not exist.

I think thats the easiest way to solve this issue.

Original comment by mbac...@gmail.com on 26 Jul 2009 at 12:20

Attachments:

GoogleCodeExporter commented 9 years ago 
The problem is that users wouldn't notice typos or invalid files in their code. 
I can't 
integrate that patch, sorry.

Original comment by wkornew...@gmail.com on 26 Jul 2009 at 12:39

GoogleCodeExporter commented 9 years ago 
What about printing an error message instead of raising the error and stopping 
the 
generation?

Original comment by mbac...@gmail.com on 26 Jul 2009 at 1:00

GoogleCodeExporter commented 9 years ago 
I think adding "logging.warning('File not found %s' % path)" after the 
"cache[path] = 
''" line produces a very noticable warning.

Original comment by mbac...@gmail.com on 26 Jul 2009 at 1:12

GoogleCodeExporter commented 9 years ago 
I don't want to add such a hacky solution. Feel free to modify it to suit your 
needs for the time being

Original comment by wkornew...@gmail.com on 31 Jul 2009 at 2:20