Move locations array to a database table (was: User personalized world clock location list)
reaneyk - December 8, 2008 - 20:00
| Project: | World Clock |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
I recently ran into a situation where I needed to allow users the have the ability to create a personal list of locations. I know development of version 2 will be under way shortly but I though I'd share what I've been working on. The code could be cleaner, but I didn't want to stray too far from the way the module was originally developed. I've attached two patches against the 6.x-1.x-dev branch as of 12/04/08.
KC
| Attachment | Size |
|---|---|
| worldclock_install_personalized_list_6.1.x-dev.patch | 12.59 KB |
| worldclock_module_personalized_list_6.1.x-dev.patch | 22.21 KB |
#1
Updated the install patch to include the update hook and fixed a typo in the module patch.
#2
KC,
thanks for your very interesting contribution.
I haven't had the chance to have a look at V2 code that pixture sent me. Anyway, I think this is the direction V2 is going to take, so I'll make sure to take your patch in consideration.
Marking as postponed now, whenever I'll branch for V2 -should happen very soon- I'll re-open this issue.
Thanks again,
Francesco
#3
Sounds good to me, thanks!
#4
I branched for the v2 today, would you please re-roll against DRUPAL-6--2 please? I suggest getting the code out of CVS so we can work together better. See how to create a patch and Checking out from the contributions repository inside the CVS handbook. Thanks for your interest in contributing!
#5
#6
KC,
I was reviewing your patches and I noticed they bring in 2 big improvements to the module: 1. they move locations to a dedicated database table and 2. introduce user personalized lists.
To make it easier to deal with these changes, it would be best ito keep your patches focused on one topic at once.
Moving locations is a very important improvement, which is not only functional to introducing user personalized lists but also to give admins the ability to add their own locations. In fact this was exactly the future pixture (the original creator of this module) was dreaming of for this module.
Are you interested in splitting this patch in two and helping getting this module to a new level of excellence?
#7
Sure, give me a couple days and I'll prepare if for the v2 CVS branch.
#8
Great, thank you!
If you are experimentation inclined, I suggest giving "git" a try to help managing your patches.
Looking forward to your new patch,
have fun, happy hacking!
Francesco
#9
flevour,
Recently I had a time to develop Worldclock new version and one of the feature I added is the one discussed in this issue. Anyway, basic implementation of new features are completed. I attached the archive for the review. It uses DB to store/load location settings, and have capability to add/delete/edit locations. It also has a feature of exporting/importing location settings as a CSV file.
All features seems to be working OK. Since I am still in the middle of development, the code is not clean and needs some brush up. If you have time, please download and test it.
Next step I am thinking is to separate date and time string for supporting different display type (vertical display, graphical display, etc.)
Thanks,
#10
Pixture,
the problem with this approach is that you are not, even minimally, leveraging the power of CVS.
Submitting the whole modified archive is very problematic because it ignores all latest improvements to code.
While I wait for KC's patch, I'll try to roll a patch against the DRUPAL-6--2 branch based on your module.
Cheers,
Francesco
#11
Hi there,
I have isolated both Pixture and reaneyk contributions and created this patch which moves all the locations to a database table.
I have tried to stick to Pixture code ideas, I have renamed a couple of variables and made a bit code refactoring. I almost completely used reaneyk patch for the install file, but I edited the hook_update_N to conform to standards (it's not good to use hook_schema inside an update function).
I'm using db_fetch_object which I prefer to db_fetch_array. This is an issue when passing a specific location to the JS code, as it expects an array and not an object. For the time being I have created a bridge function that takes an object and transforms it to an array in the correct order. Hopefully we'll have time to rewrite or replace the current worldclock.js in order to make it use more meaningful var names as loc.dst instead of constructs like "var dst = loc[2]".
Please test and review.
#12
I tested the patch and it works great for me. I also ran it though the coder module and cleaned up a few minor syntactical things.
KC
#13
Committed to v2.x-dev. Thanks everybody for their contribution and reaneyk for testing too.
I'm moving the original issue to #348757: Custom location labels and custom location lists. As there are other issues that I feel require more attention I won't be dedicating too much attention to user personalized lists. Anywya, reaneyk, whenever you got time to roll a patch I'll definitely review it.
Thanks again, let's get this module to rock!
Francesco
#14
Automatically closed -- issue fixed for two weeks with no activity.