<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104708/">http://git.reviewboard.kde.org/r/104708/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">First of all: I like this initiative very much. I have some feedback, but please understand that my general impression is VERY positive. But here goes with all the critique:
The Cell Editor docker: It should have it's content expand to fill the height of the docker. And the buttons to the left should be top aligned.
The formatting buttons: The un-merge button seems to be stuck in enabled mode, and we have fdo names for these icons and there exist oxygenversions.
The option widgets of the cell tool: You should add SpecialSpacer widgets to the bottom of the option widgets, so all the buttons remain top aligned.
Interaction design: The cells are not selectable when in anything but the cell tool. I find that annoying. It's not a new thing but now that you have the cell editor always visible and working it becomes more apparent. I wonder if you should not move the cell selection from the cell tool, to something the canvas does, if the mouse events are not picked up by the tool system. I'm not sure wether this can wait until after this has been moved to master, as I think the issue is a ui disaster (note: standard interaction design term for something that leaves the user in trouble, not as rude as it sounds) and kind of makes sheets unreleasable. If you do this, then the Cell tool becomes a "Cell Formatting" tool
Modebox instead of toolbox: I think this would be a good time to use the modebox Words uses rather than the toolbox that is more applicable to drawing applications. It's not a big change code wise: 10 lines or so. Doesn't have to be part of this branch but I really think it wold make sense to do it together.
The scroolbar focus: no objection
I'm ready to help with further (implementation) details.
</pre>
<br />
<p>- C.</p>
<br />
<p>On April 24th, 2012, 5:32 a.m., Marijn Kruisselbrink wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra.</div>
<div>By Marijn Kruisselbrink.</div>
<p style="color: grey;"><i>Updated April 24, 2012, 5:32 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This changes to UI of Calligra Sheets a bit by moving the cell editor widget out of the CellTool and instead have it as a separate docker. On top of that there are several new option widgets for the cell tool that provide common formatting options.
I've also changed the KoToolDocker a tiny bit, by changing the scroll area in it to not be able to get keyboard focus, at least for these new dockers that is preferred, I'm not sure if there are any other tool option widgets/use cases where you would want the scroll area containing the tool option widgets to be able to get keyboard focus?
Please test (also available in the tables-uirefactor-mek branch) and see if anything weird is happening since it is a fairly big change.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>libs/main/KoToolDocker.cpp <span style="color: grey">(44ce789)</span></li>
<li>sheets/CMakeLists.txt <span style="color: grey">(5355719)</span></li>
<li>sheets/Messages.sh <span style="color: grey">(e636ac1)</span></li>
<li>sheets/part/CanvasItem.h <span style="color: grey">(41acefe)</span></li>
<li>sheets/part/CanvasItem.cpp <span style="color: grey">(c5ccba3)</span></li>
<li>sheets/part/Factory.cpp <span style="color: grey">(fb612da)</span></li>
<li>sheets/part/View.h <span style="color: grey">(5322e43)</span></li>
<li>sheets/part/View.cpp <span style="color: grey">(5c11f98)</span></li>
<li>sheets/ui/ActionOptionWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>sheets/ui/ActionOptionWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>sheets/ui/CellEditorDocker.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>sheets/ui/CellEditorDocker.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>sheets/ui/CellToolBase.h <span style="color: grey">(aa3a094)</span></li>
<li>sheets/ui/CellToolBase.cpp <span style="color: grey">(33f2d29)</span></li>
<li>sheets/ui/CellToolBase_p.h <span style="color: grey">(7750a4a)</span></li>
<li>sheets/ui/CellToolBase_p.cpp <span style="color: grey">(0cba586)</span></li>
<li>sheets/ui/CellToolOptionWidget.h <span style="color: grey">(c46b7df)</span></li>
<li>sheets/ui/CellToolOptionWidget.cpp <span style="color: grey">(4a1d068)</span></li>
<li>sheets/ui/CellToolOptionWidgets.xml <span style="color: grey">(PRE-CREATION)</span></li>
<li>sheets/ui/ExternalEditor.h <span style="color: grey">(ab3ee6d)</span></li>
<li>sheets/ui/ExternalEditor.cpp <span style="color: grey">(e9f8781)</span></li>
<li>sheets/ui/LocationComboBox.h <span style="color: grey">(e666ada)</span></li>
<li>sheets/ui/LocationComboBox.cpp <span style="color: grey">(6215039)</span></li>
<li>sheets/ui/Selection.h <span style="color: grey">(baa9cdf)</span></li>
<li>sheets/ui/Selection.cpp <span style="color: grey">(316a2fc)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104708/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>