A rat-sized code review (wangxianzhu localrev 1267)
phni...@gmail.com
I'd like you to do a code review. Please review the following patch:
----------------------------------------------------------------------
r1267: (no author) | 2009-10-16 14:33:10 +0800
Add focus overlay and tab stop feature.
----------------------------------------------------------------------
=== ggadget/edit_element_base.cc
==================================================================
--- ggadget/edit_element_base.cc (revision 1266)
+++ ggadget/edit_element_base.cc (revision 1267)
@@ -141,8 +141,8 @@
delete impl_;
}
-bool EditElementBase::IsTabStop() const {
- return IsReallyEnabled();
+bool EditElementBase::IsTabStopDefault() const {
+ return true;
}
void EditElementBase::Layout() {
=== ggadget/combobox_element.h
==================================================================
--- ggadget/combobox_element.h (revision 1266)
+++ ggadget/combobox_element.h (revision 1267)
@@ -118,7 +118,7 @@
virtual double GetPixelHeight() const;
- virtual bool IsTabStop() const;
+ virtual bool IsTabStopDefault() const;
public:
static BasicElement *CreateInstance(View *view, const char *name);
=== ggadget/basic_element.cc
==================================================================
--- ggadget/basic_element.cc (revision 1266)
+++ ggadget/basic_element.cc (revision 1267)
@@ -37,6 +37,8 @@
#include "permissions.h"
#include "gadget.h"
#include "small_object.h"
+#include "canvas_utils.h"
+#include "gadget_consts.h"
namespace ggadget {
@@ -51,6 +53,7 @@
NULL),
view_(view),
mask_image_(NULL),
+ focus_overlay_(NULL),
cache_(NULL),
tag_name_(tag_name),
index_(kInvalidIndex), // Invalid until set by Elements.
@@ -81,7 +84,11 @@
cache_enabled_(false),
content_changed_(false),
draw_queued_(false),
- designer_mode_(false) {
+ designer_mode_(false),
+ show_focus_overlay_(false),
+ show_focus_overlay_set_(false),
+ tab_stop_(false),
+ tab_stop_set_(false) {
}
~Impl() {
@@ -100,6 +107,14 @@
return mask_image_ ? mask_image_->GetCanvas() : NULL;
}
+ void SetFocusOverlay(const Variant &image) {
+ DestroyImage(focus_overlay_);
+ focus_overlay_ = view_->LoadImage(image, false);
+ if (view_->IsFocused() && view_->GetFocusedElement() == owner_) {
+ QueueDraw();
+ }
+ }
+
void SetPixelWidth(double width) {
if (width >= 0.0 && (width != width_ || width_relative_)) {
AddToClipRegion(NULL);
@@ -574,6 +589,17 @@
owner_->DoDraw(target);
+ if (view_->IsFocused() && view_->GetFocusedElement() == owner_ &&
+ IsShowFocusOverlay()) {
+ if (!focus_overlay_) {
+ focus_overlay_ = view_->LoadImage(Variant(kDefaultFocusOverlay),
+ false);
+ }
+ ASSERT(focus_overlay_);
+ StretchMiddleDrawImage(focus_overlay_, target, 0, 0, width, height,
+ -1, -1, -1, -1);
+ }
+
if (cache_enabled_)
view_->EnableClipRegion(true);
}
@@ -1029,9 +1055,15 @@
Rectangle(left, top, right - left, bottom - top), owner_);
}
view_->FireEvent(&scriptable_event, onfocusin_event_);
+ if (owner_->IsShowFocusOverlay()) {
+ QueueDraw();
+ }
break;
case Event::EVENT_FOCUS_OUT:
view_->FireEvent(&scriptable_event, onfocusout_event_);
+ if (IsShowFocusOverlay()) {
+ QueueDraw();
+ }
break;
default:
ASSERT(false);
@@ -1042,12 +1074,28 @@
return result;
}
+ bool IsShowFocusOverlay() {
+ return (!show_focus_overlay_set_ && focus_overlay_) || show_focus_overlay_;
+ }
+
+ void SetShowFocusOverlay(bool show_focus_overlay) {
+ if (!show_focus_overlay_set_ ||
+ !show_focus_overlay_ != show_focus_overlay) {
+ show_focus_overlay_set_ = true;
+ show_focus_overlay_ = show_focus_overlay;
+ if (view_->IsFocused() && view_->GetFocusedElement() == owner_) {
+ QueueDraw();
+ }
+ }
+ }
+
public:
BasicElement *parent_;
BasicElement *owner_;
Elements *children_;
View *view_;
ImageInterface *mask_image_;
+ ImageInterface *focus_overlay_;
CanvasInterface *cache_;
const char *tag_name_;
size_t index_;
@@ -1120,6 +1168,10 @@
bool content_changed_ : 1;
bool draw_queued_ : 1;
bool designer_mode_ : 1;
+ bool show_focus_overlay_ : 1;
+ bool show_focus_overlay_set_ : 1;
+ bool tab_stop_ : 1;
+ bool tab_stop_set_ : 1;
};
#ifdef _DEBUG
@@ -1273,6 +1325,18 @@
kFlipNames, arraysize(kFlipNames));
// Note: don't use 'index' property until it is in the public API.
RegisterProperty("index", NewSlot(&BasicElement::GetIndex), NULL);
+ // Note: don't use 'showFocusOverlay' property until it is in the public API.
+ RegisterProperty("showFocusOverlay",
+ NewSlot(&BasicElement::IsShowFocusOverlay),
+ NewSlot(&BasicElement::SetShowFocusOverlay));
+ // Note: don't use 'focusOverlay' property until it is in the public API.
+ RegisterProperty("focusOverlay",
+ NewSlot(&BasicElement::GetFocusOverlay),
+ NewSlot(&BasicElement::SetFocusOverlay));
+ // Note: don't use 'tabStop' property until it is in the public API.
+ RegisterProperty("tabStop",
+ NewSlot(&BasicElement::IsTabStop),
+ NewSlot(&BasicElement::SetTabStop));
RegisterMethod("focus", NewSlot(&BasicElement::Focus));
RegisterMethod("killFocus", NewSlot(&BasicElement::KillFocus));
@@ -1738,6 +1802,33 @@
}
}
+Variant BasicElement::GetFocusOverlay() const {
+ return Variant(GetImageTag(impl_->focus_overlay_));
+}
+
+void BasicElement::SetFocusOverlay(const Variant &image) {
+ if (image != GetFocusOverlay()) {
+ impl_->SetFocusOverlay(image);
+ }
+}
+
+bool BasicElement::IsShowFocusOverlay() const {
+ return impl_->IsShowFocusOverlay();
+}
+
+void BasicElement::SetShowFocusOverlay(bool show_focus_overlay) {
+ impl_->SetShowFocusOverlay(show_focus_overlay);
+}
+
+bool BasicElement::IsTabStop() const {
+ return impl_->tab_stop_set_ ? impl_->tab_stop_ : IsTabStopDefault();
+}
+
+void BasicElement::SetTabStop(bool tab_stop) {
+ impl_->tab_stop_ = tab_stop;
+ impl_->tab_stop_set_ = true;
+}
+
void BasicElement::Focus() {
impl_->view_->SetFocus(this);
}
@@ -1746,7 +1837,7 @@
impl_->view_->SetFocus(NULL);
}
-bool BasicElement::IsTabStop() const {
+bool BasicElement::IsTabStopDefault() const {
return false;
}
=== ggadget/gadget_consts.h
==================================================================
--- ggadget/gadget_consts.h (revision 1266)
+++ ggadget/gadget_consts.h (revision 1267)
@@ -124,6 +124,8 @@
/** Default directory to store profiles. */
const char kDefaultProfileDirectory[] = ".google/gadgets";
+const char kDefaultFocusOverlay[] = "resource://focus_overlay.png";
+
const char kScrollDefaultBackgroundH[] = "resource://scroll_background_h.png";
const char kScrollDefaultGrippyH[] = "resource://scrollbar_grippy_h.png";
const char kScrollDefaultThumbH[] = "resource://scrollbar_u_h.png";
=== ggadget/basic_element.h
==================================================================
--- ggadget/basic_element.h (revision 1266)
+++ ggadget/basic_element.h (revision 1267)
@@ -344,7 +344,36 @@
void SetFlip(FlipMode flip);
//@}
+ //@{
/**
+ * Gets and sets the image to be displayed over the element when this
+ * element is focused. Setting a valid focus overlay image will automatically
+ * enable focus overlay (IsShowFocusOverlay() returns true).
+ */
+ Variant GetFocusOverlay() const;
+ void SetFocusOverlay(const Variant &image);
+ //@}
+
+ //@{
+ /**
+ * Gets and sets whether to show focus overlay when focused.
+ * If set true without setting SetFocusOverlay(), a default focus overlay
+ * image will be used.
+ */
+ bool IsShowFocusOverlay() const;
+ void SetShowFocusOverlay(bool show_focus_overlay);
+ //@}
+
+ //@{
+ /**
+ * Gets and sets whether this element is in the focus chain by pressing the
+ * tab key. It has no effect when the element is disabled.
+ */
+ bool IsTabStop() const;
+ void SetTabStop(bool tab_stop);
+ //@}
+
+ /**
* Checks whether this element is really visible considering transparency
* and visibility or the ancestors.
* In fact, this method should be named "IsPossiblyVisible" instead.
@@ -383,8 +412,11 @@
void KillFocus();
/**
* Checks if the element can be focused with the Tab key.
+ * This method (previously named IsTabStop()) is kept for
+ * backward-compatibility. Any value set by SetTabStop()
+ * will override the value returned from this method.
*/
- virtual bool IsTabStop() const;
+ virtual bool IsTabStopDefault() const;
public:
//@{
=== ggadget/edit_element_base.h
==================================================================
--- ggadget/edit_element_base.h (revision 1266)
+++ ggadget/edit_element_base.h (revision 1267)
@@ -47,7 +47,7 @@
virtual void DoClassRegister();
public:
- virtual bool IsTabStop() const;
+ virtual bool IsTabStopDefault() const;
virtual void Layout();
/**
=== ggadget/view.cc
==================================================================
--- ggadget/view.cc (revision 1266)
+++ ggadget/view.cc (revision 1267)
@@ -685,7 +685,7 @@
void SetFocusToFirstElement() {
if (children_.GetCount()) {
BasicElement *first = children_.GetItemByIndex(0);
- if (!first->IsTabStop())
+ if (!first->IsReallyEnabled() || !first->IsTabStop())
first = GetNextFocusElement(first);
SetFocus(first);
}
@@ -695,7 +695,7 @@
size_t count = children_.GetCount();
if (count) {
BasicElement *last = children_.GetItemByIndex(count - 1);
- if (!last->IsTabStop())
+ if (!last->IsReallyEnabled() || !last->IsTabStop())
last = GetPreviousFocusElement(last);
SetFocus(last);
}
@@ -785,7 +785,8 @@
}
BasicElement *GetFirstFocusInTree(BasicElement *current) {
- return current->IsTabStop() ? current : GetFirstFocusInSubTrees(current);
+ return current->IsReallyEnabled() && current->IsTabStop() ?
+ current : GetFirstFocusInSubTrees(current);
}
BasicElement *GetFirstFocusInSubTrees(BasicElement *current) {
@@ -808,7 +809,7 @@
BasicElement *result = GetLastFocusInSubTrees(current);
if (result)
return result;
- return current->IsTabStop() ? current : NULL;
+ return current->IsReallyEnabled() && current->IsTabStop() ? current : NULL;
}
BasicElement *GetLastFocusInSubTrees(BasicElement *current) {
@@ -2250,6 +2251,10 @@
impl_->gadget_->GetDefaultFontSize() : kDefaultFontSize;
}
+bool View::IsFocused() const {
+ return impl_->view_focused_;
+}
+
Connection *View::ConnectOnCancelEvent(Slot0<void> *handler) {
return impl_->oncancel_event_.Connect(handler);
}
=== ggadget/view.h
==================================================================
--- ggadget/view.h (revision 1266)
+++ ggadget/view.h (revision 1267)
@@ -472,6 +472,9 @@
*/
int GetDefaultFontSize() const;
+ /** Checks if the view itself is focused. */
+ bool IsFocused() const;
+
public: // Event connection methods.
Connection *ConnectOnCancelEvent(Slot0<void> *handler);
Connection *ConnectOnClickEvent(Slot0<void> *handler);
=== ggadget/combobox_element.cc
==================================================================
--- ggadget/combobox_element.cc (revision 1266)
+++ ggadget/combobox_element.cc (revision 1267)
@@ -824,7 +824,7 @@
return impl_->onchange_event_.Connect(slot);
}
-bool ComboBoxElement::IsTabStop() const {
+bool ComboBoxElement::IsTabStopDefault() const {
return impl_->edit_ != NULL;
}
=== resources/focus_overlay.png
==================================================================
Cannot display: file marked as a binary type.
Property changes on: resources/focus_overlay.png
___________________________________________________________________
Name: svn:mime-type
+application/octet-stream
=== resources/Makefile.am
==================================================================
--- resources/Makefile.am (revision 1266)
+++ resources/Makefile.am (revision 1267)
@@ -28,6 +28,7 @@
combo_arrow_down.png \
combo_arrow_over.png \
common.js \
+ focus_overlay.png \
gadget_about.xml \
ggl_about.xml \
google-gadgets.png \
This is a semiautomated message from "svkmail". Complaints or suggestions?
Mail edy...@gmail.com.
