Re: [PATCH 2 of 2 RFC] summary: add "last comment" tooltip to status icon
On 02/18/2016 05:36 PM, Angel Ezquerra wrote:
# HG changeset patch
# User ezquerra
# Date 1455813238 -3600
# Thu Feb 18 17:33:58 2016 +0100
# Branch stable
# Node ID e30cb2b8e73aa5917e811e8090c7c9f72d00d998
# Parent 8087e93914290771452ad99d6d6a7f57c7fa02c3
summary: add "last comment" tooltip to status icon
This adds a tooltip to the "number of comments" indicator, indicating who did
the last comment and also showing the first line (up to 80 characters) of that
last comment. This lets you quickly check who last commented and what the
comment was about.
Note that as with the previous changeset in this series this is RFC.
A final solution should be improved in several ways:
- Apply this same change to the changelog template
- Move the code that this change embeds in the HTML somewhere else
- Maybe the calculations could be done once and stored in the database?
Since this is the first time that I change this sort of code (I did not even
know what mako was an hour ago!) I send this as an RFC in the hopes of getting
pointers to where this could be changed more cleanly. I also worry whether this
could have some negative performance impact?
diff --git a/kallithea/templates/changelog/changelog_summary_data.html
b/kallithea/templates/changelog/changelog_summary_data.html
--- a/kallithea/templates/changelog/changelog_summary_data.html
+++ b/kallithea/templates/changelog/changelog_summary_data.html
@@ -10,6 +10,16 @@
last_cm = cm
return last_cm
+def find_last_non_empty_comment():
+"""find the last non empty comment"""
+# comments might be emtpy if they only change the status
+last_cm = None
+for cm in c.comments[cs.raw_id]:
+if not cm.text:
+continue
+last_cm = cm
+return last_cm
But that might a different one from the one giving it the color?
It seems like this will put too much different info into too little
space. I am not convinced that is a good idea.
+
def get_comment_username(cm):
return cm.author.username
%>
@@ -49,8 +59,13 @@
%if c.comments.get(cs.raw_id,[]):
+ <%last_comment_cm = find_last_non_empty_comment()%>
-
+ %if last_comment_cm is None:
+
+ %else:
+
+ %endif
I would prefer to keep the html template well structured only have one
div and let the title be conditional. I think that makes it easier to
read and maintain.
${len(c.comments[cs.raw_id])}
/Mads
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: [PATCH 1 of 2 RFC] summary: add "last status change author" tooltip to status icon
On 02/18/2016 05:36 PM, Angel Ezquerra wrote: # HG changeset patch # User ezquerra ui.username ? ;-) # Date 1455812934 -3600 # Thu Feb 18 17:28:54 2016 +0100 # Branch stable # Node ID 8087e93914290771452ad99d6d6a7f57c7fa02c3 # Parent 511aa91947fe31ae4b123a483fac7f2fb1408af2 summary: add "last status change author" tooltip to status icon This adds a tooltip to the status change icon, indicating who did the last status change. This lets you quickly check who last changed the status of each changeset. Note that this is RFC. A final solution should be improved in several ways: - Apply this same change to the changelog template Indeed. We have a lot of stupid code duplication between the summary page and the changelog view. I think the summary page should be very different from what it is now and not show the short changelog. Something like "most recent active branches" would be much more useful. Until then, I wouldn't care much about the summary page. Code sharing between for example changelog and PR and compare is much more important. - Move the code that this change embeds in the HTML somewhere else - Maybe the calculations could be done once and stored in the database? As mentioned below, we almost already get the data you are looking for in db.py . Since this is the first time that I change this sort of code (I did not even know what mako was an hour ago!) I send this as an RFC in the hopes of getting pointers to where this could be changed more cleanly. I also worry whether this could have some negative performance impact? diff --git a/kallithea/templates/changelog/changelog_summary_data.html b/kallithea/templates/changelog/changelog_summary_data.html --- a/kallithea/templates/changelog/changelog_summary_data.html +++ b/kallithea/templates/changelog/changelog_summary_data.html @@ -1,5 +1,18 @@ ## -*- coding: utf-8 -*- %if c.repo_changesets: +<% +def find_last_status_change_comment(): +"""find the last comment which changed the review state""" +last_cm = None +for cm in c.comments[cs.raw_id]: +if not cm.status_change: +continue +last_cm = cm +return last_cm + +def get_comment_username(cm): +return cm.author.username The function name is longer than the body and it says pretty much the same. Just inline it? +%> @@ -21,7 +34,14 @@ %else: - +<%last_status_change_cm = find_last_status_change_comment()%> +%if last_status_change_cm is not None: + +%endif + +%if last_status_change_cm is not None: + +%endif The approval color for the changeset comes from c.statuses. I think we should get the name of whoever gave it that color from the same place. That will avoid the extra loop that might be O(n*n) and slow. I think it will be easy to make the statuses function in db.py return the user_id too. %endif %endif /Mads ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
[PATCH 1 of 2 RFC] summary: add "last status change author" tooltip to status icon
# HG changeset patch # User ezquerra # Date 1455812934 -3600 # Thu Feb 18 17:28:54 2016 +0100 # Branch stable # Node ID 8087e93914290771452ad99d6d6a7f57c7fa02c3 # Parent 511aa91947fe31ae4b123a483fac7f2fb1408af2 summary: add "last status change author" tooltip to status icon This adds a tooltip to the status change icon, indicating who did the last status change. This lets you quickly check who last changed the status of each changeset. Note that this is RFC. A final solution should be improved in several ways: - Apply this same change to the changelog template - Move the code that this change embeds in the HTML somewhere else - Maybe the calculations could be done once and stored in the database? Since this is the first time that I change this sort of code (I did not even know what mako was an hour ago!) I send this as an RFC in the hopes of getting pointers to where this could be changed more cleanly. I also worry whether this could have some negative performance impact? diff --git a/kallithea/templates/changelog/changelog_summary_data.html b/kallithea/templates/changelog/changelog_summary_data.html --- a/kallithea/templates/changelog/changelog_summary_data.html +++ b/kallithea/templates/changelog/changelog_summary_data.html @@ -1,5 +1,18 @@ ## -*- coding: utf-8 -*- %if c.repo_changesets: +<% +def find_last_status_change_comment(): +"""find the last comment which changed the review state""" +last_cm = None +for cm in c.comments[cs.raw_id]: +if not cm.status_change: +continue +last_cm = cm +return last_cm + +def get_comment_username(cm): +return cm.author.username +%> @@ -21,7 +34,14 @@ %else: - +<%last_status_change_cm = find_last_status_change_comment()%> +%if last_status_change_cm is not None: + +%endif + +%if last_status_change_cm is not None: + +%endif %endif %endif # HG changeset patch # User ezquerra # Date 1455812934 -3600 # Thu Feb 18 17:28:54 2016 +0100 # Branch stable # Node ID 8087e93914290771452ad99d6d6a7f57c7fa02c3 # Parent 511aa91947fe31ae4b123a483fac7f2fb1408af2 summary: add "last status change author" tooltip to status icon This adds a tooltip to the status change icon, indicating who did the last status change. This lets you quickly check who last changed the status of each changeset. Note that this is RFC. A final solution should be improved in several ways: - Apply this same change to the changelog template - Move the code that this change embeds in the HTML somewhere else - Maybe the calculations could be done once and stored in the database? Since this is the first time that I change this sort of code (I did not even know what mako was an hour ago!) I send this as an RFC in the hopes of getting pointers to where this could be changed more cleanly. I also worry whether this could have some negative performance impact? diff --git a/kallithea/templates/changelog/changelog_summary_data.html b/kallithea/templates/changelog/changelog_summary_data.html --- a/kallithea/templates/changelog/changelog_summary_data.html +++ b/kallithea/templates/changelog/changelog_summary_data.html @@ -1,5 +1,18 @@ ## -*- coding: utf-8 -*- %if c.repo_changesets: +<% +def find_last_status_change_comment(): +"""find the last comment which changed the review state""" +last_cm = None +for cm in c.comments[cs.raw_id]: +if not cm.status_change: +continue +last_cm = cm +return last_cm + +def get_comment_username(cm): +return cm.author.username +%> @@ -21,7 +34,14 @@ %else: - +<%last_status_change_cm = find_last_status_change_comment()%> +%if last_status_change_cm is not None: + +%endif + +%if last_status_change_cm is not None: + +%endif %endif %endif ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
[PATCH 0 of 2 RFC] summary: add additional info to the tooltips of the status and comment icons
This series tries to add some additional info to the tooltips of the status and comment icons on the changeset summary page. This is RFC because I am not sure if this is the right way to do these changes and because if accepted the changes should also be done to the changeset list. ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
[PATCH 2 of 2 RFC] summary: add "last comment" tooltip to status icon
# HG changeset patch
# User ezquerra
# Date 1455813238 -3600
# Thu Feb 18 17:33:58 2016 +0100
# Branch stable
# Node ID e30cb2b8e73aa5917e811e8090c7c9f72d00d998
# Parent 8087e93914290771452ad99d6d6a7f57c7fa02c3
summary: add "last comment" tooltip to status icon
This adds a tooltip to the "number of comments" indicator, indicating who did
the last comment and also showing the first line (up to 80 characters) of that
last comment. This lets you quickly check who last commented and what the
comment was about.
Note that as with the previous changeset in this series this is RFC.
A final solution should be improved in several ways:
- Apply this same change to the changelog template
- Move the code that this change embeds in the HTML somewhere else
- Maybe the calculations could be done once and stored in the database?
Since this is the first time that I change this sort of code (I did not even
know what mako was an hour ago!) I send this as an RFC in the hopes of getting
pointers to where this could be changed more cleanly. I also worry whether this
could have some negative performance impact?
diff --git a/kallithea/templates/changelog/changelog_summary_data.html
b/kallithea/templates/changelog/changelog_summary_data.html
--- a/kallithea/templates/changelog/changelog_summary_data.html
+++ b/kallithea/templates/changelog/changelog_summary_data.html
@@ -10,6 +10,16 @@
last_cm = cm
return last_cm
+def find_last_non_empty_comment():
+"""find the last non empty comment"""
+# comments might be emtpy if they only change the status
+last_cm = None
+for cm in c.comments[cs.raw_id]:
+if not cm.text:
+continue
+last_cm = cm
+return last_cm
+
def get_comment_username(cm):
return cm.author.username
%>
@@ -49,8 +59,13 @@
%if c.comments.get(cs.raw_id,[]):
+ <%last_comment_cm = find_last_non_empty_comment()%>
-
+ %if last_comment_cm is None:
+
+ %else:
+
+ %endif
${len(c.comments[cs.raw_id])}
# HG changeset patch
# User ezquerra
# Date 1455813238 -3600
# Thu Feb 18 17:33:58 2016 +0100
# Branch stable
# Node ID e30cb2b8e73aa5917e811e8090c7c9f72d00d998
# Parent 8087e93914290771452ad99d6d6a7f57c7fa02c3
summary: add "last comment" tooltip to status icon
This adds a tooltip to the "number of comments" indicator, indicating who did
the last comment and also showing the first line (up to 80 characters) of that
last comment. This lets you quickly check who last commented and what the
comment was about.
Note that as with the previous changeset in this series this is RFC.
A final solution should be improved in several ways:
- Apply this same change to the changelog template
- Move the code that this change embeds in the HTML somewhere else
- Maybe the calculations could be done once and stored in the database?
Since this is the first time that I change this sort of code (I did not even
know what mako was an hour ago!) I send this as an RFC in the hopes of getting
pointers to where this could be changed more cleanly. I also worry whether this
could have some negative performance impact?
diff --git a/kallithea/templates/changelog/changelog_summary_data.html b/kallithea/templates/changelog/changelog_summary_data.html
--- a/kallithea/templates/changelog/changelog_summary_data.html
+++ b/kallithea/templates/changelog/changelog_summary_data.html
@@ -10,6 +10,16 @@
last_cm = cm
return last_cm
+def find_last_non_empty_comment():
+"""find the last non empty comment"""
+# comments might be emtpy if they only change the status
+last_cm = None
+for cm in c.comments[cs.raw_id]:
+if not cm.text:
+continue
+last_cm = cm
+return last_cm
+
def get_comment_username(cm):
return cm.author.username
%>
@@ -49,8 +59,13 @@
%if c.comments.get(cs.raw_id,[]):
+ <%last_comment_cm = find_last_non_empty_comment()%>
-
+ %if last_comment_cm is None:
+
+ %else:
+
+ %endif
${len(c.comments[cs.raw_id])}
___
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 4:13 PM, Thomas De Schampheleire < patrickdeping...@gmail.com> wrote: > > > So this means that Kallithea would start using the 'default' routing > method used by Turbogears projects? > Is this a big impact / does it require a lot of work, according to you? > Nope, just that instead of relying RoutesMiddleware to run mapper.routematch and fill environ with routes variables (routes.url, routes.route, wsgiorg.routing_args) those should be filled by RootController itself. Actually as I'm moving the routing logic from Kallithea to tgext.routes (so that Kallithea can just use it instead of implementing custom routing itself) that change is already undergoing, but I didn't have time to finish it so far. ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 12:42 PM, Alessandro Molina wrote: > > > On Thu, Feb 18, 2016 at 12:05 PM, Thomas De Schampheleire > wrote: >> >> Thanks, for basic 404 it indeed works. >> However, I discovered at least one scenario where it does not: > > > Thanks for pointing that out. > The reason is that when dispatching errors we do not pass through the > middleware stack again, so RoutesMiddleware is not executed again and the > routes dict remains the same as before. This causes our RootController to > dispatch it again to the same route as before which leads to a 404 error > again. > > I provided a work-around in > https://bitbucket.org/_amol_/kallithea-tg/commits/5ec7001cd47dece91580d8d533bacf3f0ab0d990 > but probably the "correct" solution would be to remove RoutesMiddleware and > perform the route resolution inside RootController so that the route is > resolved every time we perform a dispatch and not only when receiving a > request from the WSGI server. So this means that Kallithea would start using the 'default' routing method used by Turbogears projects? Is this a big impact / does it require a lot of work, according to you? Thanks, Thomas ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 12:05 PM, Thomas De Schampheleire < patrickdeping...@gmail.com> wrote: > > Thanks, for basic 404 it indeed works. > However, I discovered at least one scenario where it does not: > Thanks for pointing that out. The reason is that when dispatching errors we do not pass through the middleware stack again, so RoutesMiddleware is not executed again and the routes dict remains the same as before. This causes our RootController to dispatch it again to the same route as before which leads to a 404 error again. I provided a work-around in https://bitbucket.org/_amol_/kallithea-tg/commits/5ec7001cd47dece91580d8d533bacf3f0ab0d990 but probably the "correct" solution would be to remove RoutesMiddleware and perform the route resolution inside RootController so that the route is resolved every time we perform a dispatch and not only when receiving a request from the WSGI server. ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 11:24 AM, Alessandro Molina wrote: > > > On Thu, Feb 18, 2016 at 10:56 AM, Alessandro Molina > wrote: >> >> >> >> On Thu, Feb 18, 2016 at 10:26 AM, Thomas De Schampheleire >> wrote: >>> >>> More ideas? >>> >> >> I'll try to check it today before lunch. > > > https://bitbucket.org/_amol_/kallithea-tg/commits/7e27305d248627d2e15bb2e8bdc3dbc84065c094 > seems to fix the custom error pages. > Thanks, for basic 404 it indeed works. However, I discovered at least one scenario where it does not: 1. create a new empty repository (Admin->Repositories->Add repository) 2. from the new page, click Options -> Fork, accept default settings 3. from the forked repo page, click Options -> Compare Fork In the original Pylons kallithea, a custom error page with a flash is shown, with the solution above it is a real 404. The url is: http://localhost:5000/empty/compare/r...@tip...rev@tip?other_repo=empty-fork&merge=1 > Sadly I wrongly pulled from mainstream repository, so I also brought in all > the changes from the current main kallithea repository, while I just wanted > to pull your changes from the kallithea-tg repo. They merged without > conflicts, but I don't know if something broke. Content-wise it's probably fine. I guess we'll need to cleanup commits before being able to integrate it completely in Kallithea mainstream, so it's not a big problem I think. Thanks, Thomas ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
New commits on Our Own Kallithea
pullrequest: add URL changesets in txt notification email to reviewers Similar to adding the URL for each changeset in the html notification email (commit x... patrickdp committed on 2016-02-08 16:42:59branch: defaulttag: tipchangeset: bbd307cepullrequest: add URL changesets in txt notification email to reviewers Similar to adding the URL for each changeset in the html notification email (commit ) do the same for txt emails. The e-mail client presumably makes these URLs clickable. M kallithea/templates/email_templates/pull_request.txt (1 lines added, 1 lines removed) ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 10:56 AM, Alessandro Molina < alessandro.mol...@gmail.com> wrote: > > > On Thu, Feb 18, 2016 at 10:26 AM, Thomas De Schampheleire < > patrickdeping...@gmail.com> wrote: >> >> More ideas? >> >> > I'll try to check it today before lunch. > https://bitbucket.org/_amol_/kallithea-tg/commits/7e27305d248627d2e15bb2e8bdc3dbc84065c094 seems to fix the custom error pages. Sadly I wrongly pulled from mainstream repository, so I also brought in all the changes from the current main kallithea repository, while I just wanted to pull your changes from the kallithea-tg repo. They merged without conflicts, but I don't know if something broke. ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 10:26 AM, Thomas De Schampheleire < patrickdeping...@gmail.com> wrote: > > More ideas? > > I'll try to check it today before lunch. > > Btw, what exactly does this: > > error = ErrorController() > > in the root controller do? This was not obvious from the docs for me. > Standard routing in TG works by traversing the RootController attributes, so an url like /error/document would lead to RootController.error.document being executed. It should be explained by http://turbogears.readthedocs.org/en/latest/turbogears/controllers.html if there is hard to grasp let me know, I'll try to improve the documentation. > And in the kallithea port we are not using this mechanism to add > routes? How is it done instead? The routing in Kallithea-tg first looks for a routes match, in case it's not found it should revert to standard TG routing ( https://bitbucket.org/_amol_/kallithea-tg/src/8babf35fc56435cfd6f95203403226a9a2b14a98/kallithea/controllers/root.py?at=default&fileviewer=file-view-default#root.py-12:13 ) but I have to admit that I actually didn't try to mount any subcontroller in RootController so I'm not sure it's actually working in practice :D ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
Re: Turbogears2 migration: custom error pages
On Thu, Feb 18, 2016 at 8:39 AM, Alessandro Molina wrote: > > > On Wed, Feb 17, 2016 at 10:36 PM, Thomas De Schampheleire > wrote: >> >> I noticed that our custom error pages (e.g. on 404) do not work. >> >> Do you have some idea on what is wrong, or can you guide me in the >> right direction? > > > When an error is detected TurboGears replaces the request with a request for > /error/document. > And that seems to be happening as your logs contain: > > 2016-02-17 22:32:15.266 INFO [kallithea.lib.base] IP: 127.0.0.1 User: > accessed /error/document > > Might it be related to the fact that ErrorController had an empty > __before__? As in TG it is called _before probably you want to rename it as > _before if you want to throw away the behaviour provided by BaseController. I tried adding a _before with the same content (pass) as __before__ but it didn't help. > > If it still doesn't work you probably want to copy an error controller from > a quickstarted project > https://github.com/TurboGears/tg2devtools/blob/master/devtools/templates/turbogears/%2Bpackage%2B/controllers/error.py_tmpl > and mount it inside RootController through standard TG routing > https://github.com/TurboGears/tg2devtools/blob/master/devtools/templates/turbogears/%2Bpackage%2B/controllers/root.py_tmpl#L57 > just to test if it's a controller or configuration problem. I replaced our error.py with the example, and added the 'error = ErrorController()' line, but also no changes. I added a print in both 'def document', but it is never reached. I also added a debug log in the BaseController, and that is also not printed when accessing a non-existing page. More ideas? Btw, what exactly does this: error = ErrorController() in the root controller do? This was not obvious from the docs for me. And in the kallithea port we are not using this mechanism to add routes? How is it done instead? Thanks, Thomas ___ kallithea-general mailing list kallithea-general@sfconservancy.org http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
