C'est effectivement un bug de la version 0.3. Il faudra que
je refasse le code d'affichage pour faire un tri manuel,
hors de la base de donnes avant l'affichage. a sera aprs
la 0.4.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
One thing that worries me with such patches is the overhead. Right now, as far as I can tell, the patch just fixes the problem for the top level listing. This is a problem as some installations (Koumbit, namely) have only a few (ie. 4) top level projects, and it's not really relevant wether they're sorted or not. Sorting second-level projects, or all listings, in general, is the general problem, and is in direct contradiction with the chosen database structure.
This patch also introduces an API change in top_level_tree() that probably break other code... did you test this patch before submitting it? ;)
So in short, I wouldn't commit this patch right away. Also, I think the fundamental problem is the datastructure and the "project" conception. Either the timetracker shouldn't care about that or the projects should be a flatlist (ie. no hierarchy) with tags on punches (as proposed by the "timetracker 2").
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Actually, my patch does work recursively ie. not just for the top level.
As for the API change, the only thing is changed is the way the projects are ordered (ie. alphabetically ), so I don't feel like it's much of a change.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ah, ok, I see... Well, the problem then is that it now expanded the number of requests required to unfold the tree of projects. Before, it was listed in a single request, now it's listed in O(N) requests, where N is the number of projects (!).
As for the API change, yes, you are changing the API in this patch. top_level_tree() used to return an iterator (ie. a Project, ie. a Storable that is an object that can be "next()'d"):
- return new Project($where, null, null, "`path` ASC");
1. it doesn't scale: it eats up O(N) memory, where N is the number of projects (and I know the Storable object isn't exactly lightweight)
2. it completely breaks API backward compatibility. This would be better if the patch would also include changes to adapt the rest of the code (arguably acceptable) or add a wrapper function (worse).
I would much rather fix the problem at its core. The root cause of the problem is an optimisation of the database structure that made it possible to list the tree in a single SQL request. This was introduced in the 0.3 release, which was arguably a mistake, but was envisionned as a way to reduce the SQL load in rendering the project tree. The project tree used to be a traditionnal "parent = n" structure before 0.3, at which point I've implemented php/StorableTree.inc.php, as per the "Tree in SQL pattern" (http://c2.com/cgi/wiki?TreeInSql).
Now, I think the proper solution to this problem is to reorder the subtrees upon insertion/deletion of nodes. On the top of my head, the move_subtree(), save(), set_parent() and maybe find_empty_slot() would need to be hacked in StorableTree.inc.php.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Allright. An easy fix would then be to include, in the name of a child, the full name of its parent. Then the name of the child could be extracted with appropriate substr. The SQL query would naturally order everything nicely with an ORDER BY `name` at the end.
that's an interesting idea. we could "reduce" the name of the project in a string that would be sortable properly.
The only problem with this is that the algorithm is based on a fixed-width (currently 8, iirc) of the "subpaths". So there would be a limit to the number of characters we could get out of the name...
Patches welcome. :P
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Logged In: YES
user_id=246797
C'est effectivement un bug de la version 0.3. Il faudra que
je refasse le code d'affichage pour faire un tri manuel,
hors de la base de donnes avant l'affichage. a sera aprs
la 0.4.
Logged In: YES
user_id=590072
Originator: NO
J'ai écrit une patch:
--- phptimetracker-0.5/phptimetracker/php/Project.php 2005-09-05 16:51:46.000000000 -0400
+++ phptimetracker-0.5.new/phptimetracker/php/Project.php 2007-01-24 00:08:41.000000000 -0500
@@ -114,7 +114,7 @@
*
* @param bool $favorites_only
*/
- function top_level_tree($customer = null) {
+ function top_level_tree($customer = null, $parentpath = '') {
/* ugly copy-paste from above */
// generate a proper customer array
if (is_null($customer)) {
@@ -127,8 +127,15 @@
} else {
$where = "`customer` = '".addslashes($customer)."'";
}
- return new Project($where, null, null, "`path` ASC");
-
+
+ $length = strlen($parentpath) + KEYLENGTH;
+ $tree = new Project("LENGTH(`path`) = '$length' AND `path` LIKE '$parentpath%' AND " . $where, null, null, "`name` ASC");
+ $ret = array();
+ while ($tree->next()) {
+ $ret[] = $tree;
+ $ret = array_merge($ret, Project::top_level_tree($customer, $tree->get('path')));
+ }
+ return $ret;
}
/**
@@ -148,8 +155,10 @@
* @param bool $favorites_only
*/
function walk_tree($callback, $customer, $depth = 0, $favorites_only = true) {
- $leafs = Project::top_level_tree($customer);
- while ($leafs->next()) {
+ $array_leafs = Project::top_level_tree($customer);
+ // manual sort
+
+ foreach ($array_leafs as $leafs) {
if (!$favorites_only ||
User_Project::is_favorite_project($leafs->get('id'))) {
/*
Logged In: YES
user_id=246797
Originator: NO
One thing that worries me with such patches is the overhead. Right now, as far as I can tell, the patch just fixes the problem for the top level listing. This is a problem as some installations (Koumbit, namely) have only a few (ie. 4) top level projects, and it's not really relevant wether they're sorted or not. Sorting second-level projects, or all listings, in general, is the general problem, and is in direct contradiction with the chosen database structure.
This patch also introduces an API change in top_level_tree() that probably break other code... did you test this patch before submitting it? ;)
So in short, I wouldn't commit this patch right away. Also, I think the fundamental problem is the datastructure and the "project" conception. Either the timetracker shouldn't care about that or the projects should be a flatlist (ie. no hierarchy) with tags on punches (as proposed by the "timetracker 2").
Logged In: NO
Actually, my patch does work recursively ie. not just for the top level.
As for the API change, the only thing is changed is the way the projects are ordered (ie. alphabetically ), so I don't feel like it's much of a change.
Logged In: YES
user_id=246797
Originator: NO
Ah, ok, I see... Well, the problem then is that it now expanded the number of requests required to unfold the tree of projects. Before, it was listed in a single request, now it's listed in O(N) requests, where N is the number of projects (!).
As for the API change, yes, you are changing the API in this patch. top_level_tree() used to return an iterator (ie. a Project, ie. a Storable that is an object that can be "next()'d"):
- return new Project($where, null, null, "`path` ASC");
With your patch, it would now return an array:
+ $ret = array();
+ while ($tree->next()) {
+ $ret[] = $tree;
+ $ret = array_merge($ret, Project::top_level_tree($customer,
$tree->get('path')));
+ }
+ return $ret;
I'm sorry, but that is not acceptable:
1. it doesn't scale: it eats up O(N) memory, where N is the number of projects (and I know the Storable object isn't exactly lightweight)
2. it completely breaks API backward compatibility. This would be better if the patch would also include changes to adapt the rest of the code (arguably acceptable) or add a wrapper function (worse).
I would much rather fix the problem at its core. The root cause of the problem is an optimisation of the database structure that made it possible to list the tree in a single SQL request. This was introduced in the 0.3 release, which was arguably a mistake, but was envisionned as a way to reduce the SQL load in rendering the project tree. The project tree used to be a traditionnal "parent = n" structure before 0.3, at which point I've implemented php/StorableTree.inc.php, as per the "Tree in SQL pattern" (http://c2.com/cgi/wiki?TreeInSql).
Now, I think the proper solution to this problem is to reorder the subtrees upon insertion/deletion of nodes. On the top of my head, the move_subtree(), save(), set_parent() and maybe find_empty_slot() would need to be hacked in StorableTree.inc.php.
Logged In: NO
Allright. An easy fix would then be to include, in the name of a child, the full name of its parent. Then the name of the child could be extracted with appropriate substr. The SQL query would naturally order everything nicely with an ORDER BY `name` at the end.
E.g. if I get the following project architecture:
MyParentA
- MyChildA
- MyChildB
MyParentB
- MyChildA
In the tables I would have:
MyParentA
MyParentAMyChildA
MyParentAMyChildB
MyParentB
MyParentBMyChildA
Logged In: YES
user_id=246797
Originator: NO
that's an interesting idea. we could "reduce" the name of the project in a string that would be sortable properly.
The only problem with this is that the algorithm is based on a fixed-width (currently 8, iirc) of the "subpaths". So there would be a limit to the number of characters we could get out of the name...
Patches welcome. :P