On Mon, Aug 8, 2016 at 1:18 PM, Akshay Joshi <akshay.jo...@enterprisedb.com> wrote:
> > > On Mon, Aug 8, 2016 at 5:33 PM, Dave Page <dp...@pgadmin.org> wrote: > >> Hi >> >> On Mon, Aug 8, 2016 at 11:39 AM, Akshay Joshi < >> akshay.jo...@enterprisedb.com> wrote: >> >>> Hi All >>> >>> I have fixed the RM#1250 "Collection node counts". To fix this RM I need >>> to do following changes >>> >>> - Move "check_precondition" function from module's view class to >>> global level within that python file itself, so that module class will >>> use >>> it. >>> - Modified "get_nodes" function of each module's class, run the sql >>> query to count the number of objects and pass the count to >>> "generate_browser_collection_node" function to display the >>> collection count. >>> - Reuse SQL queries which is used to fetch nodes. Make that query as >>> inner query like "SELECT count(*) FROM( <node's> query ) AS >>> collection_count". For that I'll have to remove semicolon's from some of >>> the SQL queries. >>> >>> One case is not handled with this patch and that is on "Refresh" of >>> collection node, count is not updated. If user refresh the parent node then >>> it will be updated. I'll create a separate RM for that. >>> >> >> Sorry Akshay, but I really don't like the way you've done this. It seems >> like an unnecessarily large patch, and if I'm reading the patch correctly, >> it doubles the amount of SQL queries run against the database when >> navigating the tree, and introduces race conditions where the count >> displayed could be different from the actual number of nodes. >> >> I was expecting to see this implemented by watching for tree events (e.g. >> 'added' and 'removed') and using those events to update the label on the >> parent node, if that node is a collection. That should just be a few lines, >> and should be correct at all times right? >> > > With current implementation children's of any collection node will be > fetched/added when user will expand that collection node, in that case we > will update the label once the node gets expanded. For example initially we > will show "Databases" and when it gets expanded then we will update it to > "Databases (5)". > Right, but you also need to allow for removal and addition of children when the node is already expanded, and refreshes. Plus my other comments are still valid I believe - race condition, double the SQL and a very large change late in the beta cycle which isn't ideal. > >> >>> >>> Attached is the patch file. Please review it and let me know the review >>> comments. >>> >>> -- >>> *Akshay Joshi* >>> *Principal Software Engineer * >>> >>> >>> >>> *Phone: +91 20-3058-9517 <%2B91%2020-3058-9517>Mobile: +91 976-788-8246* >>> >>> >>> -- >>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) >>> To make changes to your subscription: >>> http://www.postgresql.org/mailpref/pgadmin-hackers >>> >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > > -- > *Akshay Joshi* > *Principal Software Engineer * > > > > *Phone: +91 20-3058-9517 <%2B91%2020-3058-9517>Mobile: +91 976-788-8246* > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
