Issue 6348058: Adds functionality to the matrix and clip widget. (Closed)
DescriptionAdds functionality to the matrix and clip widget.
Committed: https://code.google.com/p/skia/source/detail?r=4453
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 18
Patch Set 3 : #
Total comments: 16
Patch Set 4 : #Patch Set 5 : #
Total comments: 36
Patch Set 6 : #Patch Set 7 : #
Total comments: 4
MessagesTotal messages: 12
https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkCanvasWidget.h File debugger/QT/SkCanvasWidget.h (right): https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkCanvasWidget.h#ne... debugger/QT/SkCanvasWidget.h:61: QString* getCurrentMatrix(); just return the SkMatrix* and do the conversion in the caller https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkCanvasWidget.h#ne... debugger/QT/SkCanvasWidget.h:128: QString fCurClip[4]; no need to have these if the caller does the work. https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkDebuggerGUI.cpp File debugger/QT/SkDebuggerGUI.cpp (right): https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkDebuggerGUI.cpp#n... debugger/QT/SkDebuggerGUI.cpp:193: remove https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkDebuggerGUI.cpp#n... debugger/QT/SkDebuggerGUI.cpp:233: remove extra lines https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkInspectorWidget.cpp File debugger/QT/SkInspectorWidget.cpp (right): https://codereview.appspot.com/6348058/diff/1/debugger/QT/SkInspectorWidget.c... debugger/QT/SkInspectorWidget.cpp:82: void SkInspectorWidget::setMatrixText(QString* matrixValues) { just have this take the SkMatrix* and do the conversion here.
Sign in to reply to this message.
http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkCanvasWidget.h File debugger/QT/SkCanvasWidget.h (right): http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkCanvasWidget.h#new... debugger/QT/SkCanvasWidget.h:61: QString* getCurrentMatrix(); On 2012/06/29 20:37:50, DerekS wrote: > just return the SkMatrix* and do the conversion in the caller Done. http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkCanvasWidget.h#new... debugger/QT/SkCanvasWidget.h:128: QString fCurClip[4]; On 2012/06/29 20:37:50, DerekS wrote: > no need to have these if the caller does the work. Done. http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkDebuggerGUI.cpp File debugger/QT/SkDebuggerGUI.cpp (right): http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkDebuggerGUI.cpp#ne... debugger/QT/SkDebuggerGUI.cpp:193: On 2012/06/29 20:37:50, DerekS wrote: > remove Done. http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkDebuggerGUI.cpp#ne... debugger/QT/SkDebuggerGUI.cpp:233: On 2012/06/29 20:37:50, DerekS wrote: > remove extra lines Done. http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkInspectorWidget.cpp File debugger/QT/SkInspectorWidget.cpp (right): http://codereview.appspot.com/6348058/diff/1/debugger/QT/SkInspectorWidget.cp... debugger/QT/SkInspectorWidget.cpp:82: void SkInspectorWidget::setMatrixText(QString* matrixValues) { On 2012/06/29 20:37:50, DerekS wrote: > just have this take the SkMatrix* and do the conversion here. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkCanvasWidget.h File debugger/QT/SkCanvasWidget.h (right): http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkCanvasWidget.h#... debugger/QT/SkCanvasWidget.h:132: QString fCurClip[4]; you can delete these now. http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... File debugger/QT/SkInspectorWidget.cpp (right): http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.cpp:14: // NOTE(chudy): Keeps the inspector widget fully expanded. : QWidget(parent) , fHorizontalLayout(this) , fTabWidget() , fDetailTab() , fDetailLayout(&fDetailTab) { http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.cpp:61: SkInspectorWidget::~SkInspectorWidget() {} you need to clean up all your pointers here or you will leak memory. http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.cpp:64: return fDetailText->toHtml(); is this value ever called? It appears that the string is never populated. http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.cpp:68: return fOverviewText->toHtml(); is this value ever called? It appears that the string is never populated. http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.cpp:92: QVBoxLayout* SkInspectorWidget::currentMatrix() { instead of current matrix this seems to be more of a GUI setup function. http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.cpp:93: fMatrixLabel = new QLabel(this); this will cause a memory leak as you never cleaned up the previous fMatrixLabel http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget.h File debugger/QT/SkInspectorWidget.h (right): http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.h:49: void setDisabled(bool toggle) { Calling the parameter toggle which doesn't give any indication to what you should pass to disable it. Instead call it something like isDisabled http://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget... debugger/QT/SkInspectorWidget.h:79: QHBoxLayout* fDetailLayout; QHboxLayout fHorizontalLayout;
Sign in to reply to this message.
Next version, mostly translation and resizing stuff. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkCanvasWidget.h File debugger/QT/SkCanvasWidget.h (right): https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkCanvasWidget.h... debugger/QT/SkCanvasWidget.h:132: QString fCurClip[4]; On 2012/07/02 13:34:47, DerekS wrote: > you can delete these now. Done. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... File debugger/QT/SkInspectorWidget.cpp (right): https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:14: // NOTE(chudy): Keeps the inspector widget fully expanded. On 2012/07/02 13:34:47, DerekS wrote: > : QWidget(parent) > , fHorizontalLayout(this) > , fTabWidget() > , fDetailTab() > , fDetailLayout(&fDetailTab) { Done. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:61: SkInspectorWidget::~SkInspectorWidget() {} On 2012/07/02 13:34:47, DerekS wrote: > you need to clean up all your pointers here or you will leak memory. Don't have any pointers left to clean up! =) https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:64: return fDetailText->toHtml(); On 2012/07/02 13:34:47, DerekS wrote: > is this value ever called? It appears that the string is never populated. Removed. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:68: return fOverviewText->toHtml(); On 2012/07/02 13:34:47, DerekS wrote: > is this value ever called? It appears that the string is never populated. Removed. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:92: QVBoxLayout* SkInspectorWidget::currentMatrix() { On 2012/07/02 13:34:47, DerekS wrote: > instead of current matrix this seems to be more of a GUI setup function. Done. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:93: fMatrixLabel = new QLabel(this); On 2012/07/02 13:34:47, DerekS wrote: > this will cause a memory leak as you never cleaned up the previous fMatrixLabel Done. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidget.h File debugger/QT/SkInspectorWidget.h (right): https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.h:49: void setDisabled(bool toggle) { On 2012/07/02 13:34:47, DerekS wrote: > Calling the parameter toggle which doesn't give any indication to what you > should pass to disable it. Instead call it something like isDisabled Done. https://codereview.appspot.com/6348058/diff/5001/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.h:79: QHBoxLayout* fDetailLayout; On 2012/07/02 13:34:47, DerekS wrote: > QHboxLayout fHorizontalLayout; Done.
Sign in to reply to this message.
https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.cpp File debugger/QT/SkCanvasWidget.cpp (right): https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:21: fBitmap = new SkBitmap(); remove the pointer from fBitmap. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:30: fTransform = new QTransform(); none of these should be pointers. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:37: you should initialize index to 0 https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:46: fBitmap = new SkBitmap(); if it isn't a pointer just call fBitmap.setConfig() as it will take care of calling reset() to delete the old pixels. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:53: delete fCanvas; shouldn't need to change any of this if the SkBitmap isn't deleted. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:85: int r = (color >> 16) & 0xff; we have macros for this called SkColorGetR(color) https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:117: x = event->globalX() - startX; would a better name for x be deltaX? https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkInspectorWidge... File debugger/QT/SkInspectorWidget.cpp (right): https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:101: if(i%3 == 2) fMatrixLayout.addLayout(&fMatrixRow[i/3]); the if statement should be on it's own line and have braces according to the style doc.
Sign in to reply to this message.
Fixed comments + crashing, added ToDos and zooming in / out. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.cpp File debugger/QT/SkCanvasWidget.cpp (right): https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:21: fBitmap = new SkBitmap(); On 2012/07/02 18:27:26, DerekS wrote: > remove the pointer from fBitmap. Done. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:30: fTransform = new QTransform(); On 2012/07/02 18:27:26, DerekS wrote: > none of these should be pointers. Done. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:37: On 2012/07/02 18:27:26, DerekS wrote: > you should initialize index to 0 I initialize it after loadPicture to the length of the command vector. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:46: fBitmap = new SkBitmap(); On 2012/07/02 18:27:26, DerekS wrote: > if it isn't a pointer just call fBitmap.setConfig() as it will take care of > calling reset() to delete the old pixels. Done. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:53: delete fCanvas; On 2012/07/02 18:27:26, DerekS wrote: > shouldn't need to change any of this if the SkBitmap isn't deleted. Done. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:85: int r = (color >> 16) & 0xff; On 2012/07/02 18:27:26, DerekS wrote: > we have macros for this called SkColorGetR(color) Done. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:117: x = event->globalX() - startX; On 2012/07/02 18:27:26, DerekS wrote: > would a better name for x be deltaX? Replaced with SkPoints. https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkInspectorWidge... File debugger/QT/SkInspectorWidget.cpp (right): https://codereview.appspot.com/6348058/diff/3009/debugger/QT/SkInspectorWidge... debugger/QT/SkInspectorWidget.cpp:101: if(i%3 == 2) fMatrixLayout.addLayout(&fMatrixRow[i/3]); On 2012/07/02 18:27:26, DerekS wrote: > the if statement should be on it's own line and have braces according to the > style doc. Done.
Sign in to reply to this message.
https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.cpp File debugger/QT/SkCanvasWidget.cpp (right): https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:17: * when the widget is resized which makes ugly clipping occur on translations. can you show me what this ugly clipping is? I'm not sure I follow how changing the bitmap size will make the clipping look better. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:37: scaleFactor = 1; you also need to initialize the index variable https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:42: SkCanvasWidget::~SkCanvasWidget() {} you need to delete the fCanvas and fDevice here. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:50: width = event->size().width(); why store the width and height if you never use them? https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:94: /* NOTE(chudy): Updated style sheet without a background specified to you don't need to put the note notation on normal comments. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:95: * draw over our SkCanvas. */ it draws under not over. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:97: QString stylezz("QWidget {border: 1px solid #cccccc; background-color: #"); drop the zz from stylezz https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:99: stylezz.append(QString::number(g,16)); space between params https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:111: p0.fY = event->globalY(); SkPoint eventPoint = SkPoint::Make(SkFloatToScalar(event->globalX()), ...); delta = eventPoint - p0; p0 = eventPoint; https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:119: p0.fY = event->globalY(); p0.set(SkFloatToScalar(event->globalX()), ...); https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:122: delta.fY = 0; delta.set(0,0); https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:138: QImage image((uchar *)fBitmap.getPixels(), fBitmap.width(), fBitmap.height(), QImage::Format_ARGB32_Premultiplied); break into 2 lines https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:141: painter.drawImage(origin,image); add space between params https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:150: scaleFactor += (event->delta()/120) * 2; not sure what this is doing so please comment it. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:162: fDebugCanvas->drawTo(fCanvas, index+1); why do we need the +1? can't they both just be zero based or one based? https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h File debugger/QT/SkCanvasWidget.h (right): https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h... debugger/QT/SkCanvasWidget.h:146: // I think this initializes to 0,0 looking at SkPoint there is no default constructor so you will need to set the values in your constructor. p0.set(0,0); delta.set(0,0); https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h... debugger/QT/SkCanvasWidget.h:154: int width; prefix your member variables with f.
Sign in to reply to this message.
Updated https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.cpp File debugger/QT/SkCanvasWidget.cpp (right): https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:17: * when the widget is resized which makes ugly clipping occur on translations. On 2012/07/03 13:12:26, DerekS wrote: > can you show me what this ugly clipping is? I'm not sure I follow how changing > the bitmap size will make the clipping look better. If the bitmap is bounded to 100x100 and is inside of a border that size you can't tell that the picture got clipped to those bounds. But as soon as you pan over even a couple pixels you will notice that the picture got cut off at those bounds since its still 100x100, even if its left corner is now -50,-50 instead of 0,0 so half the picture is cut off. Not sure if this explained it so come over if you want an example https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:37: scaleFactor = 1; On 2012/07/03 13:12:26, DerekS wrote: > you also need to initialize the index variable Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:42: SkCanvasWidget::~SkCanvasWidget() {} On 2012/07/03 13:12:26, DerekS wrote: > you need to delete the fCanvas and fDevice here. Done. Also deleting fDebugCanvas https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:50: width = event->size().width(); On 2012/07/03 13:12:26, DerekS wrote: > why store the width and height if you never use them? Removed. It was used to get the bottom right pixels color until I realized I could just use the width / height functions from bitmap https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:94: /* NOTE(chudy): Updated style sheet without a background specified to On 2012/07/03 13:12:26, DerekS wrote: > you don't need to put the note notation on normal comments. Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:95: * draw over our SkCanvas. */ On 2012/07/03 13:12:26, DerekS wrote: > it draws under not over. I fixed the comment to hopefully be clearer https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:97: QString stylezz("QWidget {border: 1px solid #cccccc; background-color: #"); On 2012/07/03 13:12:26, DerekS wrote: > drop the zz from stylezz Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:99: stylezz.append(QString::number(g,16)); On 2012/07/03 13:12:26, DerekS wrote: > space between params Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:111: p0.fY = event->globalY(); On 2012/07/03 13:12:26, DerekS wrote: > SkPoint eventPoint = SkPoint::Make(SkFloatToScalar(event->globalX()), ...); > > delta = eventPoint - p0; > p0 = eventPoint; Switched over to the new way of setting SkPoint arguements but am confused as to the purpose of SkFloatToScalar() function. event->globals() returns an int, what is the purpose of having it as an SkScalar type. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:119: p0.fY = event->globalY(); On 2012/07/03 13:12:26, DerekS wrote: > p0.set(SkFloatToScalar(event->globalX()), ...); Same comment as above. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:122: delta.fY = 0; On 2012/07/03 13:12:26, DerekS wrote: > delta.set(0,0); Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:138: QImage image((uchar *)fBitmap.getPixels(), fBitmap.width(), fBitmap.height(), QImage::Format_ARGB32_Premultiplied); On 2012/07/03 13:12:26, DerekS wrote: > break into 2 lines Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:141: painter.drawImage(origin,image); On 2012/07/03 13:12:26, DerekS wrote: > add space between params Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:141: painter.drawImage(origin,image); On 2012/07/03 13:12:26, DerekS wrote: > add space between params Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:150: scaleFactor += (event->delta()/120) * 2; On 2012/07/03 13:12:26, DerekS wrote: > not sure what this is doing so please comment it. Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:162: fDebugCanvas->drawTo(fCanvas, index+1); On 2012/07/03 13:12:26, DerekS wrote: > why do we need the +1? can't they both just be zero based or one based? I don't know. I need to figure out why it breaks otherwise. Added a TODO. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h File debugger/QT/SkCanvasWidget.h (right): https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h... debugger/QT/SkCanvasWidget.h:146: // I think this initializes to 0,0 On 2012/07/03 13:12:26, DerekS wrote: > looking at SkPoint there is no default constructor so you will need to set the > values in your constructor. > > p0.set(0,0); > delta.set(0,0); Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h... debugger/QT/SkCanvasWidget.h:146: // I think this initializes to 0,0 On 2012/07/03 13:12:26, DerekS wrote: > looking at SkPoint there is no default constructor so you will need to set the > values in your constructor. > > p0.set(0,0); > delta.set(0,0); Done. https://codereview.appspot.com/6348058/diff/9007/debugger/QT/SkCanvasWidget.h... debugger/QT/SkCanvasWidget.h:154: int width; On 2012/07/03 13:12:26, DerekS wrote: > prefix your member variables with f. Done.
Sign in to reply to this message.
Updated with Skia canvas transform instead of Qt Transform
Sign in to reply to this message.
lgtm with those 2 changes. https://codereview.appspot.com/6348058/diff/1010/debugger/QT/SkCanvasWidget.cpp File debugger/QT/SkCanvasWidget.cpp (right): https://codereview.appspot.com/6348058/diff/1010/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:19: * Is there anyway remove this. https://codereview.appspot.com/6348058/diff/1010/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:125: fP0 = eventPoint; why reset the initial point on each move? It seems to make more sense to keep the initial point and do fTransform = eventPoint - fP0; if that works then you don't even need to keep the delta as a member variable.
Sign in to reply to this message.
Pushed as 4453 https://codereview.appspot.com/6348058/diff/1010/debugger/QT/SkCanvasWidget.cpp File debugger/QT/SkCanvasWidget.cpp (right): https://codereview.appspot.com/6348058/diff/1010/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:19: * Is there anyway On 2012/07/03 15:54:36, DerekS wrote: > remove this. Done. https://codereview.appspot.com/6348058/diff/1010/debugger/QT/SkCanvasWidget.c... debugger/QT/SkCanvasWidget.cpp:125: fP0 = eventPoint; On 2012/07/03 15:54:36, DerekS wrote: > why reset the initial point on each move? It seems to make more sense to keep > the initial point and do > > fTransform = eventPoint - fP0; > > if that works then you don't even need to keep the delta as a member variable. Removed.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
