Merge lp:~reynolds.chin/glance/adaptive_index_listing into lp:glance/diablo

Proposed by Reynolds Chin
Status: Needs review
Proposed branch: lp:~reynolds.chin/glance/adaptive_index_listing
Merge into: lp:glance/diablo
Diff against target: 126 lines (+77/-4)
1 file modified
bin/glance (+77/-4)
To merge this branch: bzr merge lp:~reynolds.chin/glance/adaptive_index_listing
Reviewer Review Type Date Requested Status
Glance Drivers Pending
Brian Waldon Pending
Review via email: mp+74067@code.launchpad.net

Description of the change

the "glance index" provides a more style of listing images.
1. the number of image indexes is "adapted" to the size of the console, while you still can customize the number of output per page through the parameter --limit
2. users "need not" to press the "enter key" to confirm YES or NO.
3. seamless output, you will never see the prompt message by scrolling the console's bar upward.

usage: glance index (for adaptive listing)
        glance index --limit 15 (for customized listing)

To post a comment you must log in.
212. By Reynolds Chin

improvement:
1. you never see prompt message by scrolling the screen upward
2. fully use of rows in listing

213. By Reynolds Chin

remove wrong if-statement

214. By Reynolds Chin

compensation for row while the header is false

Revision history for this message
Kevin L. Mitchell (klmitch) wrote :

First, you should be aware that merging is now done through Gerrit; you should review the workflow at http://wiki.openstack.org/GerritWorkflow for information on that.

Now, on to your patch. My first major concern is that the constructs you use may not be 100% portable. I see that, in most places, you've used try/except blocks to try to mitigate that, but you use termios in getch() without a try/except, and termios is UNIX-specific. I also see some style problems—for instance, you removed a blank line (line 8), so there is only one line between the DEFAULT_COL_LIMIT assignment and the def catch_error(). Also, I think you assign to the wrong variable on line 34 of the patch, and your indentation in the blocks starting at line 40 seems off. The space before the comma on line 72 should probably be removed, and you don't have enough blank lines between def getch() and def user_response().

Have you run this through PEP8? That will highlight certain style problems I may not be seeing here. You should also run the test suite and make sure all the tests pass, particularly the functional tests that hit "glance index". (The test suite also, incidentally, runs PEP8; your patch has to get a clean bill of health from PEP8 before the automated system will allow it to be merged.)

215. By Reynolds Chin

bin/glance get a clean bill of health from PEP8

Unmerged revisions

215. By Reynolds Chin

bin/glance get a clean bill of health from PEP8

214. By Reynolds Chin

compensation for row while the header is false

213. By Reynolds Chin

remove wrong if-statement

212. By Reynolds Chin

improvement:
1. you never see prompt message by scrolling the screen upward
2. fully use of rows in listing

211. By Reynolds Chin

the "glance index" provides a more style of listing images.
1. the number of image indexes is "adapted" to the size of the console,
   while you still can customize the number of output per page through the parameter --limit
2. users need not to press the "enter key" to confirm YES or NO.
3. the prompt "Fetch next page?" wouldn't be concatenated to the output

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bin/glance'
--- bin/glance 2011-08-30 21:48:11 +0000
+++ bin/glance 2011-09-07 03:29:23 +0000
@@ -48,6 +48,8 @@
4848
49SUCCESS = 049SUCCESS = 0
50FAILURE = 150FAILURE = 1
51DEFAULT_ROW_LIMIT = 128
52DEFAULT_COL_LIMIT = 128
5153
5254
53#TODO(sirp): make more of the actions use this decorator55#TODO(sirp): make more of the actions use this decorator
@@ -70,6 +72,77 @@
70 return wrap72 return wrap
7173
7274
75def getTerminalSize():
76 """
77 determine the number of Rows and Cols of a console
78 code inpired by Johannes WeiB (http://tux4u.de/)
79 """
80 def ioctl_GWINSZ(fd):
81 try:
82 import fcntl
83 import termios
84 import struct
85 import os
86 ncol_nrow = struct.unpack('hh', \
87 fcntl.ioctl(fd, termios.TIOCGWINSZ, '1234'))
88 except:
89 return None
90 return ncol_nrow
91 nrow_ncol = ioctl_GWINSZ(0) or ioctl_GWINSZ(1) or ioctl_GWINSZ(2)
92 if not nrow_ncol:
93 try:
94 fd = os.open(os.ctermid(), os.O_RDONLY)
95 nrow_ncol = ioctl_GWINSZ(fd)
96 os.close(fd)
97 except:
98 pass
99 if not nrow_ncol:
100 try:
101 import subprocess
102 cmd = "stty size"
103 p = subprocess.Popen(cmd.split(), stdout=subprocess.PIPE)
104 p.wait()
105 if p.returncode == 0:
106 return tuple(int(x) for x in p.stdout.read().split())
107 except:
108 nrow_ncol = (DEFAULT_COL_LIMIT, DEFAULT_ROW_LIMIT)
109 return int(nrow_ncol[0]), int(nrow_ncol[1])
110
111
112def getch():
113 """
114 getchar
115 """
116 import sys
117 import tty
118 import termios
119 fd = sys.stdin.fileno()
120 old = termios.tcgetattr(fd)
121 try:
122 tty.setraw(fd)
123 return sys.stdin.read(1)
124 finally:
125 termios.tcsetattr(fd, termios.TCSADRAIN, old)
126
127
128def user_response(prompt, quit='q'):
129 """
130 interactie question dialog with user without pressing the return.
131 Press any to confirm, while you can map a key for "no"
132
133 :param prompt: question/statement to present to user (string)
134 :param quit: it is the key mapped to NO. The key 'q' is defaultly mapped.
135 """
136 print prompt,
137 key = getch()
138 if key == quit:
139 return False
140 else:
141 sys.stdout.write('\b' * len(prompt))
142 sys.stdout.flush()
143 return True
144
145
73def get_percent_done(image):146def get_percent_done(image):
74 try:147 try:
75 pct_done = image['size'] * 100 / int(image['expected_size'])148 pct_done = image['size'] * 100 / int(image['expected_size'])
@@ -423,7 +496,7 @@
423 """Driver function for images_index"""496 """Driver function for images_index"""
424 parameters = {497 parameters = {
425 "filters": filters,498 "filters": filters,
426 "limit": limit,499 "limit": limit or getTerminalSize()[0],
427 }500 }
428501
429 optional_kwargs = ['marker', 'sort_key', 'sort_dir']502 optional_kwargs = ['marker', 'sort_key', 'sort_dir']
@@ -454,7 +527,7 @@
454 image['size'])527 image['size'])
455528
456 if not options.force and len(images) == limit and \529 if not options.force and len(images) == limit and \
457 not user_confirm("Fetch next page?", True):530 not user_response("press any key to continue, or press 'q' to quit"):
458 return SUCCESS531 return SUCCESS
459532
460 parameters['marker'] = images[-1]['id']533 parameters['marker'] = images[-1]['id']
@@ -474,7 +547,7 @@
474fields are treated as image metadata properties"""547fields are treated as image metadata properties"""
475 client = get_client(options)548 client = get_client(options)
476 filters = get_image_filters_from_args(args)549 filters = get_image_filters_from_args(args)
477 limit = options.limit550 limit = options.limit or getTerminalSize()[0] - 3
478 marker = options.marker551 marker = options.marker
479 sort_key = options.sort_key552 sort_key = options.sort_key
480 sort_dir = options.sort_dir553 sort_dir = options.sort_dir
@@ -990,7 +1063,7 @@
990 metavar="TOKEN", default=None,1063 metavar="TOKEN", default=None,
991 help="Authentication token to use to identify the "1064 help="Authentication token to use to identify the "
992 "client to the glance server")1065 "client to the glance server")
993 parser.add_option('--limit', dest="limit", metavar="LIMIT", default=10,1066 parser.add_option('--limit', dest="limit", metavar="LIMIT", default=None,
994 type="int", help="Page size to use while "1067 type="int", help="Page size to use while "
995 "requesting image metadata")1068 "requesting image metadata")
996 parser.add_option('--marker', dest="marker", metavar="MARKER",1069 parser.add_option('--marker', dest="marker", metavar="MARKER",

Subscribers

People subscribed via source and target branches

to all changes: