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

#3355: Fix order of external files #25

Closed
wants to merge 1 commit into from
Closed

#3355: Fix order of external files #25

wants to merge 1 commit into from

Conversation

franzliedke
Copy link
Contributor

This now takes into account the time at which they were registered and compares these when the priority of two elements is the same.

Trac ticket: http://trac.elgg.org/ticket/3355

This might seem a little complex at first, but it really is a simple solution that works properly and is clean - I don't like create_function(). The function is still simple enough that noone needs to worry about performance or something like that...

… same priority.

This now takes into account the time at which they were registered and compares these when the priority of two elements is the same.
@brettp
Copy link
Member

brettp commented Apr 19, 2011

Hey Franz, thanks for the pull request. I see that you're doing this based on load order and not registration order. Do you think this is more intuitive than registration?

@franzliedke
Copy link
Contributor Author

Maybe the key "load_order" is not obvious enough. It is increased every time something is passed to elgg_load_external_file(), so - to the best of my understanding - this should be registration order.

@brettp
Copy link
Member

brettp commented Apr 20, 2011

There are two functions in question: elgg_register_external_file() and elgg_load_external_file(). Callers set the priority in the register function, and only files that have been registered can be loaded. It's possible to call load() in a different order to register(). Your changes make it so the order load() is called determines the order if priority is the same. I was thinking order should be determined by the order register() was called.

@ewinslow
Copy link
Contributor

I could see it going both ways, but I think for consistency I agree with brett -- registration order should determine default final order. It's too bad we can't just have a sort function that maintains original order in case of ties...

@franzliedke
Copy link
Contributor Author

Ah, yes, of course you are right, Brett. Don't know why I didn't discover the right function right away. Will push a fix later on.

@franzliedke
Copy link
Contributor Author

Actually, when looking at it again. We only display loaded files, so this should only take loaded files into account.

Also, I cannot easily add a reg_order parameter since files can also be registered by using the elgg_load_external_file() function, which would potentially mess things up...

@brettp
Copy link
Member

brettp commented Apr 21, 2011

My initial thought was to add a static var to elgg_register_external_file() that increments the default priority each time a file is registered without an explicit priority. Didn't follow the logic all the way through, but it seemed like a simple fix. It would still work for loaded vs unloaded files and their positions.

@brettp
Copy link
Member

brettp commented Apr 28, 2011

@lie2815 - Did you have any ideas for this (if you're still interested in working on it)? If not I'll implement something...just let me know.

@franzliedke
Copy link
Contributor Author

Whoops, I am. I just kind of forgot. If some time this week is good enough, I'll do it.

@franzliedke
Copy link
Contributor Author

Gosh, right now I'm glad I didn't make it into GSoC. I am swamped, can you take care of this? I hope this is at least a little start that may help you along...

@brettp
Copy link
Member

brettp commented May 2, 2011

Yeup np. Should be fairly easy. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants