From 74c5ab19fd69d3c030f53e5b64493d548b74f916 Mon Sep 17 00:00:00 2001 From: "zachr@google.com" Date: Wed, 7 Aug 2013 18:06:39 +0000 Subject: add ui for mutli-rebaselining R=epoger@google.com Review URL: https://codereview.chromium.org/22580004 git-svn-id: http://skia.googlecode.com/svn/trunk@10618 2bbb7eff-a529-9590-31e7-b0007b416f81 --- tools/skpdiff/diff_viewer.js | 64 +++++++++++++++++++----- tools/skpdiff/skpdiff_server.py | 107 +++++++++++++++++++++++++++------------- tools/skpdiff/viewer.html | 99 +++++++++++++++++++++---------------- tools/skpdiff/viewer_style.css | 62 ++++++++++++++++++++++- 4 files changed, 242 insertions(+), 90 deletions(-) diff --git a/tools/skpdiff/diff_viewer.js b/tools/skpdiff/diff_viewer.js index e7156b359f..9c33f84fa1 100644 --- a/tools/skpdiff/diff_viewer.js +++ b/tools/skpdiff/diff_viewer.js @@ -76,7 +76,11 @@ directive('swapImg', function() { }; }); -function DiffListController($scope, $http, $timeout) { +function DiffListController($scope, $http, $location, $timeout, $parse) { + // Detect if we are running the web server version of the viewer. If so, we set a flag and + // enable some extra functionality of the website for rebaselining. + $scope.isDynamic = ($location.protocol() == "http" || $location.protocol() == "https"); + // Label each kind of differ for the sort buttons. $scope.differs = [ { @@ -90,12 +94,20 @@ function DiffListController($scope, $http, $timeout) { // Puts the records within AngularJS scope $scope.records = SkPDiffRecords.records; + // Keep track of the index of the last record to change so that shift clicking knows what range + // of records to apply the action to. + $scope.lastSelectedIndex = undefined; + // Indicates which diff metric is used for sorting $scope.sortIndex = 1; // Called by the sort buttons to adjust the metric used for sorting $scope.setSortIndex = function(idx) { $scope.sortIndex = idx; + + // Because the index of things has most likely changed, the ranges of shift clicking no + // longer make sense from the user's point of view. We reset it to avoid confusion. + $scope.lastSelectedIndex = undefined; }; // A predicate for pulling out the number used for sorting @@ -103,30 +115,58 @@ function DiffListController($scope, $http, $timeout) { return record.diffs[$scope.sortIndex].result; }; - // Flash status indicators on the rows, and then remove them so the style can potentially be + // Flash status indicator on the page, and then remove it so the style can potentially be // reapplied later. - $scope.flashRowStatus = function(success, record) { + $scope.flashStatus = function(success) { var flashStyle = success ? "success-flash" : "failure-flash"; var flashDurationMillis = success ? 500 : 800; // Store the style in the record. The row will pick up the style this way instead of through // index because index can change with sort order. - record.cssClasses = flashStyle; + $scope.statusClass = flashStyle; // The animation cannot be repeated unless the class is removed the element. $timeout(function() { - record.cssClasses = ""; + $scope.statusClass = ""; }, flashDurationMillis); - } + }; + + $scope.selectedRebaseline = function(index, event) { + // Retrieve the records in the same order they are displayed. + var recordsInOrder = $parse("records | orderBy:sortingDiffer")($scope); + + // If the user is shift clicking, apply the last tick/untick to all elements in between this + // record, and the last one they ticked/unticked. + if (event.shiftKey && $scope.lastSelectedIndex !== undefined) { + var currentAction = recordsInOrder[index].isRebaselined; + var smallerIndex = Math.min($scope.lastSelectedIndex, index); + var largerIndex = Math.max($scope.lastSelectedIndex, index); + for (var recordIndex = smallerIndex; recordIndex <= largerIndex; recordIndex++) { + recordsInOrder[recordIndex].isRebaselined = currentAction; + } + $scope.lastSelectedIndex = index; + } + else + { + $scope.lastSelectedIndex = index; + } + + }; - $scope.setHashOf = function(imagePath, record) { - $http.post("/set_hash", { - "path": imagePath + $scope.commitRebaselines = function() { + // Gather up all records that have the rebaseline set. + var rebaselines = []; + for (var recordIndex = 0; recordIndex < $scope.records.length; recordIndex++) { + if ($scope.records[recordIndex].isRebaselined) { + rebaselines.push($scope.records[recordIndex].testPath); + } + } + $http.post("/commit_rebaselines", { + "rebaselines": rebaselines }).success(function(data) { - $scope.flashRowStatus(data.success, record); + $scope.flashStatus(data.success); }).error(function() { - $scope.flashRowStatus(false, record); + $scope.flashStatus(false); }); - }; } diff --git a/tools/skpdiff/skpdiff_server.py b/tools/skpdiff/skpdiff_server.py index 14035403a2..053291af9a 100755 --- a/tools/skpdiff/skpdiff_server.py +++ b/tools/skpdiff/skpdiff_server.py @@ -99,6 +99,8 @@ def download_gm_image(image_name, image_path, hash_val): @param image_path Path to place the image. @param hash_val The hash value of the image. """ + if hash_val is None: + return # Separate the test name from a image name image_match = IMAGE_FILENAME_RE.match(image_name) @@ -174,14 +176,18 @@ class GMInstance: - image_name = the GM test name and config - expected_hash = the current expected hash value - actual_hash = the actual hash value + - is_rebaselined = True if actual_hash is what is currently in the expected + results file, False otherwise. """ def __init__(self, device_name, image_name, - expected_hash, actual_hash): + expected_hash, actual_hash, + is_rebaselined): self.device_name = device_name self.image_name = image_name self.expected_hash = expected_hash self.actual_hash = actual_hash + self.is_rebaselined = is_rebaselined class ExpectationsManager: @@ -228,13 +234,14 @@ class ExpectationsManager: # Invoke skpdiff with our downloaded images and place its results in the # temporary directory. - self.skpdiff_output_path = os.path.join(image_output_dir, + self._skpdiff_output_path = os.path.join(image_output_dir, 'skpdiff_output.json') skpdiff_cmd = SKPDIFF_INVOKE_FORMAT.format(self._skpdiff_path, - self.skpdiff_output_path, + self._skpdiff_output_path, expected_image_dir, actual_image_dir) os.system(skpdiff_cmd) + self._load_skpdiff_output() def _get_expectations(self): @@ -267,11 +274,25 @@ class ExpectationsManager: with open(updated_file_path, 'rb') as updated_file: updated_contents = updated_file.read() + # Read the expected results on disk to determine what we've + # already rebaselined. + commited_contents = None + with open(expected_file_path, 'rb') as expected_file: + commited_contents = expected_file.read() + # Find all expectations that did not match. expected_diff = differ.GenerateDiffDictFromStrings( expected_contents, updated_contents) + # Generate a set of images that have already been rebaselined + # onto disk. + rebaselined_diff = differ.GenerateDiffDictFromStrings( + expected_contents, + commited_contents) + + rebaselined_set = set(rebaselined_diff.keys()) + # The name of the device corresponds to the name of the folder # we are in. device_name = os.path.basename(root) @@ -280,7 +301,18 @@ class ExpectationsManager: for image_name, hashes in expected_diff.iteritems(): self._expectations.append( GMInstance(device_name, image_name, - hashes['old'], hashes['new'])) + hashes['old'], hashes['new'], + image_name in rebaselined_set)) + + def _load_skpdiff_output(self): + """Loads the results of skpdiff and annotates them with whether they + have already been rebaselined or not. The resulting data is store in + self.skpdiff_records.""" + self.skpdiff_records = None + with open(self._skpdiff_output_path, 'rb') as skpdiff_output_file: + self.skpdiff_records = json.load(skpdiff_output_file)['records'] + for record in self.skpdiff_records: + record['isRebaselined'] = self.image_map[record['baselinePath']][1].is_rebaselined def _download_expectation_images(self, expected_image_dir, actual_image_dir): @@ -346,23 +378,36 @@ class ExpectationsManager: # Write it out to disk using gm_json to keep the formatting consistent. gm_json.WriteToFile(expectations, json_path) - def use_hash_of(self, image_path): - """Determine the hash of the image at the path using the records, and - set it as the expected hash for its device and image config. + def commit_rebaselines(self, rebaselines): + """Sets the expected results file to use the hashes of the images in + the rebaselines list. If a expected result image is not in rebaselines + at all, the old hash will be used. - @param image_path The path of the image as it was stored in the output - of skpdiff_path + @param rebaselines A list of image paths to use the hash of. """ + # Reset all expectations to their old hashes because some of them may + # have been set to the new hash by a previous call to this function. + for expectation in self._expectations: + expectation.is_rebaselined = False + self._set_expected_hash(expectation.device_name, + expectation.image_name, + expectation.expected_hash) + + # Take all the images to rebaseline + for image_path in rebaselines: + # Get the metadata about the image at the path. + is_actual, expectation = self.image_map[image_path] - # Get the metadata about the image at the path. - is_actual, expectation = self.image_map[image_path] + expectation.is_rebaselined = is_actual + expectation_hash = expectation.actual_hash if is_actual else\ + expectation.expected_hash - expectation_hash = expectation.actual_hash if is_actual else\ - expectation.expected_hash + # Write out that image's hash directly to the expected results file. + self._set_expected_hash(expectation.device_name, + expectation.image_name, + expectation_hash) - # Write out that image's hash directly to the expected results file. - self._set_expected_hash(expectation.device_name, expectation.image_name, - expectation_hash) + self._load_skpdiff_output() class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler): @@ -410,7 +455,15 @@ class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler): self.send_response(200) self.send_header('Content-type', MIME_TYPE_MAP['json']) self.end_headers() - self.wfile.write(self.server.skpdiff_output_json) + + # Add JSONP padding to the JSON because the web page expects it. It + # expects it because it was designed to run with or without a web + # server. Without a web server, the only way to load JSON is with + # JSONP. + skpdiff_records = self.server.expectations_manager.skpdiff_records + self.wfile.write('var SkPDiffRecords = ') + json.dump({'records': skpdiff_records}, self.wfile) + self.wfile.write(';') return # Attempt to send static asset files first. @@ -427,10 +480,11 @@ class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler): self.send_error(404) def do_POST(self): - if self.path == '/set_hash': + if self.path == '/commit_rebaselines': content_length = int(self.headers['Content-length']) request_data = json.loads(self.rfile.read(content_length)) - self.server.expectations_manager.use_hash_of(request_data['path']) + rebaselines = request_data['rebaselines'] + self.server.expectations_manager.commit_rebaselines(rebaselines) self.send_response(200) self.send_header('Content-type', 'application/json') self.end_headers() @@ -442,23 +496,11 @@ class SkPDiffHandler(BaseHTTPServer.BaseHTTPRequestHandler): def run_server(expectations_manager, port=8080): - # Preload the skpdiff results file. This is so we can perform some - # processing on it. - skpdiff_output_json = '' - with open(expectations_manager.skpdiff_output_path, 'rb') as records_file: - skpdiff_output_json = records_file.read() - # It's important to parse the results file so that we can make a set of # images that the web page might request. - skpdiff_records = json.loads(skpdiff_output_json)['records'] + skpdiff_records = expectations_manager.skpdiff_records image_set = get_image_set_from_skpdiff(skpdiff_records) - # Add JSONP padding to the JSON because the web page expects it. It expects - # it because it was designed to run with or without a web server. Without a - # web server, the only way to load JSON is with JSONP. - skpdiff_output_json = ('var SkPDiffRecords = ' + - json.dumps({'records': skpdiff_records}) + ';') - # Do not bind to interfaces other than localhost because the server will # attempt to serve files relative to the root directory as a last resort # before 404ing. This means all of your files can be accessed from this @@ -466,7 +508,6 @@ def run_server(expectations_manager, port=8080): server_address = ('127.0.0.1', port) http_server = BaseHTTPServer.HTTPServer(server_address, SkPDiffHandler) http_server.image_set = image_set - http_server.skpdiff_output_json = skpdiff_output_json http_server.expectations_manager = expectations_manager print('Navigate thine browser to: http://{}:{}/'.format(*server_address)) http_server.serve_forever() diff --git a/tools/skpdiff/viewer.html b/tools/skpdiff/viewer.html index 46c5871ae1..f4b8fd7fc9 100644 --- a/tools/skpdiff/viewer.html +++ b/tools/skpdiff/viewer.html @@ -15,50 +15,63 @@ us to use a webserver. -->
diff --git a/tools/skpdiff/viewer_style.css b/tools/skpdiff/viewer_style.css index 81b4f906c9..e172667061 100644 --- a/tools/skpdiff/viewer_style.css +++ b/tools/skpdiff/viewer_style.css @@ -19,6 +19,60 @@ thead > tr > td { border: none; } +button { + font-family: Verdana; + font-weight: 900; +} + +input[type="checkbox"] { + -webkit-appearance: none; + -moz-appearance: none; + width: 20px; + height:20px; + padding: 2px; + background: #EEE; + border-radius: 2px; + box-shadow: inset 0 0 8px -2px black; +} + +input[type="checkbox"]:checked { + background: #a9db80; + background: -webkit-linear-gradient(top, #00b7ea 0%,#009ec3 100%); +} + +input[type="checkbox"]:active { + background: #a9db80; + background: -webkit-linear-gradient(top, #009ec3 0%,#00b7ea 100%); +} + +input[type="checkbox"].lastselected { + padding: 0; + border: 2px solid #009ec3; + box-shadow: inset 0 0 8px -2px black, 0 0 6px black; +} + +.commit { + position: absolute; + top: 8px; + right: 8px; +} + +.commit > button { + border: 1px solid #00687F; + border-radius: 4px; + padding: 8px; + color: white; + text-shadow: 0 0 4px black; + box-shadow: 0 0 8px black; + background: #a9db80; + background: -webkit-linear-gradient(top, #a9db80 0%,#96c56f 100%); +} + +.commit > button:active { + background: #96c56f; + background: -webkit-linear-gradient(top, #96c56f 0%,#a9db80 100%); +} + .gm-image { border: 1px dotted black; } @@ -27,6 +81,10 @@ thead > tr > td { border: 1px dashed black; } +.selected { + background: #ffff88; +} + .left-image { float: right; } @@ -68,13 +126,13 @@ thead > tr > td { .result { padding: 8px; cursor: default; - opacity: 0.7; + -webkit-filter: grayscale(30%) } .result:hover { border: 2px dotted #DDD; padding: 6px; - opacity: 1.0; + -webkit-filter: grayscale(0) } .result-0 { -- cgit v1.2.3