#141 Don't hide results in kojiweb
Merged 7 years ago by mikem. Opened 7 years ago by xning.
https://github.com/xning/koji.git issue222-patch2  into  master

move link to the bottom
Xibo Ning • 7 years ago  
add 'Show abbreviated results' also to bottom
Xibo Ning • 7 years ago  
fix the link to full results
Xibo Ning • 7 years ago  
low the limits to 4 line and 400 chars
Xibo Ning • 7 years ago  
lower the length limits and put the expansion link at the postscript end
Xibo Ning • 7 years ago  
remove the escaped instance variable and let TaskResultLine to add postscript
Xibo Ning • 7 years ago  
fix hence the abbreviated result always with the postscript
Xibo Ning • 7 years ago  
add docstring and replace func name by a good one.
Xibo Ning • 7 years ago  
Fix as mikem review result.
Xibo Ning • 7 years ago  
fix issue 222: Don't hide results in kojiweb
Xibo Ning • 7 years ago  
www/kojiweb/index.py
file modified
+5
@@ -670,6 +670,11 @@

          values['result'] = None

          values['excClass'] = None

  

+     full_result_text, abbr_result_text = kojiweb.util.task_result_to_html(

+         values['result'], values['excClass'], abbr_postscript='...')

+     values['full_result_text'] = full_result_text

+     values['abbr_result_text'] = abbr_result_text

+ 

      output = server.listTaskOutput(task['id'])

      output.sort(_sortByExtAndName)

      values['output'] = output

www/kojiweb/taskinfo.chtml
file modified
+37 -28
@@ -393,22 +393,20 @@

      <tr>

        <th>Result</th>

        <td>

-         <a href="#" collapse" id="toggle-result" style="display: none;">Show results</a>

-         <div id="result">

-         #if $excClass

-           <pre>

-           #if $hasattr($result, 'faultString')

- $cgi.escape($result.faultString.strip())

-           #else

- ${excClass.__name__}: $cgi.escape($str($result))

-           #end if

-           </pre>

-         #elif $isinstance($result, dict)

-         $printMap($result)

-         #else

-         $printValue('', $result)

-         #end if

+       #if $abbr_result_text

+         <div id="abbr-result">

+         $abbr_result_text

+         </div>

+         <div id="full-result">

+         $full_result_text

          </div>

+         <a href="#" collapse" id="toggle-abbreviated-result" style="display: none;">Show abbreviated results</a>

+         <a href="#" collapse" id="toggle-full-result" style="display: none;">Show complete results</a>

+       #else

+          <div id="result">

+         $full_result_text

+         </div>

+       #end if

        </td>

      </tr>

      <tr>
@@ -430,25 +428,36 @@

      </tr>

    </table>

  

+ #if $abbr_result_text

    <script type="text/javascript">

      (function() {

-        var r = document.getElementById('result'),

-        t = document.getElementById('toggle-result');

-        r.style.display = 'none';

-        t.style.display = 'block';

-        t.text = 'Show result';

-        t.onclick = function(e) {

-          if(r.style.display == 'none') {

-            r.style.display = 'block';

-            t.text = 'Hide result';

-            }

+        var abbr = document.getElementById('abbr-result');

+        var full = document.getElementById('full-result');

+        var link_to_show_abbr = document.getElementById('toggle-abbreviated-result');

+        var link_to_show_full = document.getElementById('toggle-full-result');

+        full.style.display = 'none';

+        abbr.style.display = 'block'

+        link_to_show_full.style.display = 'inline'

+        link_to_show_abbr.style.display = 'none'

+        var trigger = function(e) {

+          if (link_to_show_abbr.style.display == 'none') {

+            link_to_show_abbr.style.display = 'inline'

+            link_to_show_full.style.display = 'none'

+            abbr.style.display = 'none';

+            full.style.display = 'block'

+          }

           else {

-            r.style.display = 'none';

-            t.text = 'Show result';

+            link_to_show_abbr.style.display = 'none'

+            link_to_show_full.style.display = 'inline'

+            abbr.style.display = 'block';

+            full.style.display = 'none'

           }

           return false;

         };

+ 

+        link_to_show_full.onclick = trigger

+        link_to_show_abbr.onclick = trigger

      })();

    </script>

- 

+ #end if

  #include "includes/footer.chtml"

www/lib/kojiweb/util.py
file modified
+251
@@ -31,6 +31,7 @@

  from socket import sslerror as socket_sslerror

  from xmlrpclib import ProtocolError

  from xml.parsers.expat import ExpatError

+ import cgi

  

  class NoSuchException(Exception):

      pass
@@ -597,3 +598,253 @@

      else:

          str = "An error has occurred while processing your request."

      return str, level

+ 

+ 

+ class TaskResultFragment(object):

+     """Represent an HTML fragment composed from texts and tags.

+  

+     This class permits us to compose HTML fragment by the default

+     composer method or self-defined composer function.

+ 

+     Public attributes:

+         - text

+         - size

+         - need_escape

+         - begin_tag

+         - eng_tag

+         - composer

+         - empty_str_placeholder

+     """

+     def __init__(self, text='', size=None, need_escape=None, begin_tag='',

+                      end_tag='', composer=None, empty_str_placeholder=None):

+         self.text = text

+         if size is None:

+             self.size = len(text)

+         else:

+             self.size = size

+         self.need_escape = need_escape

+         self.begin_tag = begin_tag

+         self.end_tag = end_tag

+         if composer is None:

+             self.composer = self.default_composer

+         else:

+             self.composer = lambda length=None: composer(self, length)

+         if empty_str_placeholder is None:

+             self.empty_str_placeholder = '...'

+         else:

+             self.empty_str_placeholder = empty_str_placeholder

+ 

+     def default_composer(self, length=None):

+         import cgi

+         if length is None:

+             text = self.text

+         else:

+             text = self.text[:length]

+         if self.need_escape:

+             text = cgi.escape(text)

+         if self.size > 0 and text == '':

+             text = self.empty_str_placeholder

+         return '%s%s%s' % (self.begin_tag, text, self.end_tag)

+ 

+ 

+ class TaskResultLine(object):

+     """Represent an HTML line fragment.

+ 

+     This class permits us from several TaskResultFragment instances

+     to compose an HTML fragment that ends with a line break. You

+     can use the default composer method or give a self-defined version.

+ 

+     Public attributes:

+         - fragments

+         - need_escape

+         - begin_tag

+         - end_tag

+         - composer

+     """

+     def __init__(self, fragments=None, need_escape=None, begin_tag='',

+                      end_tag='<br />', composer=None):

+         if fragments is None:

+             self.fragments = []

+         else:

+             self.fragments = fragments

+ 

+         self.need_escape = need_escape

+         self.begin_tag = begin_tag

+         self.end_tag = end_tag

+         if composer is None:

+             self.composer = self.default_composer

+         else:

+ 

+             def composer_wrapper(length=None, postscript=None):

+                 return composer(self, length, postscript)

+ 

+             self.composer = composer_wrapper

+         self.size=self._size()

+ 

+     def default_composer(self, length=None, postscript=None):

+         import cgi

+         line_text = ''

+         size = 0

+         if postscript is None:

+             postscript = ''

+ 

+         for fragment in self.fragments:

+             if length is None:

+                 line_text += fragment.composer()

+             else:

+                 if size >= length: break

+                 remainder_size = length - size

+                 line_text += fragment.composer(remainder_size)

+                 size += fragment.size

+ 

+         if self.need_escape:

+             line_text = cgi.escape(line_text)

+ 

+         return '%s%s%s%s' % (self.begin_tag, line_text, postscript, self.end_tag)

+ 

+     def _size(self):

+         return sum([fragment.size for fragment in self.fragments])

+ 

+ 

+ def _parse_value(key, value, sep=', '):

+     _str = None

+     begin_tag = ''

+     end_tag = ''

+     need_escape = True

+     if key in ('brootid', 'buildroot_id'):

+         _str = str(value)

+         begin_tag = '<a href="buildrootinfo?buildrootID=%s">' % _str

+         end_tag = '</a>'

+         need_escape = False

+     elif isinstance(value, list):

+         _str = sep.join([str(val) for val in value])

+     elif isinstance(value, dict):

+         _str = sep.join(['%s=%s' % ((n == '' and "''" or n), v)

+                              for n, v in value.items()])

+     else:

+         _str = str(value)

+     if _str is None:

+         _str = ''

+ 

+     return TaskResultFragment(text=_str, need_escape=need_escape,

+                                   begin_tag=begin_tag, end_tag=end_tag)

+ 

+ def task_result_to_html(result=None, exc_class=None,

+                             max_abbr_lines=None, max_abbr_len=None,

+                             abbr_postscript=None):

+     """convert the result to a mutiple lines HTML fragment

+ 

+     Args:

+         result: task result. Default is empty string.

+         exc_class: Exception raised when access the task result.

+         max_abbr_lines: maximum abbreviated result lines. Default is 11.

+         max_abbr_len: maximum abbreviated result length. Default is 512.

+ 

+     Returns:

+         Tuple of full result and abbreviated result.

+     """

+     default_max_abbr_result_lines = 5

+     default_max_abbr_result_len = 400

+ 

+     if max_abbr_lines is None:

+         max_abbr_lines = default_max_abbr_result_lines

+     if max_abbr_len is None:

+         max_abbr_len = default_max_abbr_result_len

+ 

+     postscript_fragment = TaskResultFragment(

+         text='...', end_tag='</a>',

+         begin_tag='<a href="#" collapse" %s %s>' % (

+             'id="toggle-full-result"',

+             'style="display: none;text-decoration:none;"'))

+ 

+     if abbr_postscript is None:

+         abbr_postscript = postscript_fragment.composer()

+     elif isinstance(abbr_postscript, TaskResultFragment):

+         abbr_postscript = abbr_postscript.composer()

+     elif isinstance(abbr_postscript, str):

+         abbr_postscript = abbr_postscript

+     else:

+         abbr_postscript = '...'

+ 

+     if not abbr_postscript.startswith(' '):

+         abbr_postscript = ' %s' % abbr_postscript

+ 

+     full_ret_str = ''

+     abbr_ret_str = ''

+     lines = []

+ 

+     def _parse_properties(props):

+         return ', '.join([v is not None and '%s=%s' % (n, v) or str(n)

+                               for n, v in props.items()])

+ 

+     if exc_class:

+         if hasattr(result, 'faultString'):

+             _str = result.faultString.strip()

+         else:

+             _str = "%s: %s" % (exc_class.__name__, str(result))

+         fragment = TaskResultFragment(text=_str, need_escape=True)

+         line = TaskResultLine(fragments=[fragment],

+                                   begin_tag='<pre>', end_tag='</pre>')

+         lines.append(line)

+     elif isinstance(result, dict):

+ 

+         def composer(line, length=None, postscript=None):

+             if postscript is None:

+                 postscript = ''

+             key_fragment = line.fragments[0]

+             val_fragment = line.fragments[1]

+             if length is None:

+                 return '%s%s = %s%s%s' % (line.begin_tag, key_fragment.composer(),

+                                             val_fragment.composer(), postscript,

+                                             line.end_tag)

+             first_part_len = len('%s = ') + key_fragment.size

+             remainder_len = length - first_part_len

+             if remainder_len < 0: remainder_len = 0

+ 

+             return '%s%s = %s%s%s' % (

+                 line.begin_tag, key_fragment.composer(),

+                 val_fragment.composer(remainder_len), postscript, line.end_tag)

+ 

+         for k, v in result.items():

+             if k == 'properties':

+                 _str = "properties = %s" % _parse_properties(v)

+                 fragment = TaskResultFragment(text=_str)

+                 line = TaskResultLine(fragments=[fragment], need_escape=True)

+             elif k != '__starstar':

+                 val_fragment = _parse_value(k, v)

+                 key_fragment = TaskResultFragment(text=k, need_escape=True)

+                 line = TaskResultLine(fragments=[key_fragment, val_fragment],

+                                           need_escape=False, composer=composer)

+             lines.append(line)

+     else:

+         if result is not None:

+             fragment = _parse_value('', result)

+             line = TaskResultLine(fragments=[fragment])

+             lines.append(line)

+ 

+     if not lines:

+         return full_ret_str, abbr_ret_str

+ 

+     total_lines = len(lines)

+     full_result_len = sum([line.size for line in lines])

+     total_abbr_lines = 0

+     total_abbr_len = 0

+ 

+     for line in lines:

+         line_len = line.size

+         full_ret_str += line.composer()

+ 

+         if total_lines < max_abbr_lines and full_result_len < max_abbr_len:

+             continue

+         if total_abbr_lines >= max_abbr_lines or total_abbr_len >= max_abbr_len:

+             continue

+ 

+         if total_abbr_len + line_len >= max_abbr_len:

+             remainder_abbr_len = max_abbr_len - total_abbr_len

+             abbr_ret_str += line.composer(remainder_abbr_len, postscript=abbr_postscript)

+         else:

+             abbr_ret_str += line.composer()

+         total_abbr_lines += 1

+         total_abbr_len += line_len

+ 

+     return full_ret_str, abbr_ret_str

no initial comment

The taskinfo page hides the result value by replacing it with the text "Show result" that expands to the full result via javascript. This was done because sometimes the result can be a big ugly mess. However, for many failed builds the result is a single useful line of text that should not be obscured.

This patch is smarter about collapsing the result. When the results is no more than 10 lines and length is no more than 512 chars, it will just show the full result. Otherwise, it will show first 10 lines or first 512 chars.

Let's lower those limits. I think 5 lines or 400 chars should be about right (10 lines is quite a lot of screen space in context).

The expansion code doesn't appear to be working. I see the abbreviated results, but there is no way to expand them. The previous patch you showed me was very close. I think just the two changes I recommended on top of it would do the trick:
https://github.com/mikem23/koji-playground/commits/issue222-patch2

1 new commit added

  • low the limits to 4 line and 400 chars
7 years ago

Have lowed the limits as your advice, mikem.

For the expansion code, I don't find all changes for taskinfo.chtml file in koji-playgroup issue222-patch2 branch. And mikem, I test this PR by python code and this chtml file. And you see, this is HTML produced, and you can try it in here.

It seems you have opted to have "..." be the expansion link instead of the more explicit "show complete result" text.

The immediate problem with this is that the code does not include the "..." when displaying a dictionary result. This is what I was seeing before.

For other result types, the "..." is present and does expand.

However, even in that case it's a bit hard to see that the "..." is a link. The change in color is not so apparent with just three small dots.

I think the simplest thing to do here is bring back the "show complete result text, but place it at the bottom, as I suggested earlier:

https://github.com/mikem23/koji-playground/commits/issue222-patch2

Have replaced the '...' by 'Show complete results' in the link to show the full results and placed the link at the bottom.

@mikem the patch in the https://github.com/mikem23/koji-playground/commits/issue222-patch2 lacks the commit, that's why you see the expand codes not work for dict type results.

But it's no matter now since we placed the link to show the complete results at the bottom of the taskinfo page.

Here is the demo that you can verify the expand code. Do this make sense?

BTW, this demo shows a dict type results.

1 new commit added

  • fix the link to full results
7 years ago

Demo that displays a long results.

@mikem if you think we should place the link to show the abbreviated results at the bottom, pls add a comment here, I will fix the patch soon.

Would it increase usability if we add 'Show abbreviated results' also to bottom? I'll display long listings and then I've to scroll back to hide it again. Not sure if it would be really usable, as I don't think, that I'll be re-hiding content after I've expanded it.

@mikem hi.

What do you think about @tkopecek' proposal?

Both at the bottom? Sounds fine to me. I think I suggested similar earlier

rebased

7 years ago

Have fixed the PR and rebased it.

I don't see link at the bottom now. I've 'Show complete results' at the bottom, but 'Show abbreviated results' on top.

1 new commit added

  • move link to the bottom
7 years ago

Commit 1a11849 fixes this pull-request

Pull-Request has been merged by mikem@redhat.com

7 years ago