Update of patch #1054 (project wesnoth):
Status: None => In Progress
_______________________________________________________
Follow-up Comment #2:
A nice design which looks good. I do have some comments and answers to your
questions. Sorry I will nitpick a bit but it's my pet project ;-)
- It doesn't compile for me due to a missing comma here:
"
character_offset_()
+ history_()
"
- Some general C++ remarks I found in your code:
- When a function is const mark it as such.
- Also use const for parameters when applicable and send const objects as
a
preference this avoids creating a copy of the object (We had performance
decrements in parts of the code due to the extra coping. It won't be a
bottleneck here, but I consider it good style to always use a const
reference
if the object is a constant.)
- A bit of a nitpick; C++ uses 0 instead of NULL.
- I find this really ugly a getter and setter with the same name:
"
+ void enabled(bool e) {enabled_ = e;}
+ bool enabled() {return enabled_;}
"
Please use set_enabled for the setter and most of the time I use
set_enabled(const bool enabled = true) { enabled_ = enabled; }
in that case you can't use the same name for getter and setter.
- For the ttext_box::handle_key_up_arrow() and
ttext_box::handle_key_down_arrow()
set handled to true when there is a history enabled. At the moment the
handled
is ignored but it will be used for chaining keyboard events later.
- When I press the down arrow I get and empty line pressing the up arrow
afterwards
doesn't return the disappeared line, not sure whether this is really user
friendly.
- Since you also don't like the pointers why did you use
std::map<std::string, std::vector<std::string>*> instead of
std::map<std::string, std::vector<std::string> > for
ttext_history::history_map
- You wrote the code rather nice and generic, it would be nice if you can
extent
the code to write the history in the preferences. In that case a maximum
number
of remembered items would also be nice, as you already noticed yourself.
(This can be a parameter in addon.cfg). Then the history can be saved
between
games which really makes the history useful in this case. It can also move
the
ttext_history::history_map to the preferences as you noted yourself.
- I think the hardcoded saving of the list is oke for now, not sure what a
better
way would be. (Maybe add a test for retval_ == tbutton::OK.)
- A bit of nitpicking; Since this is part of a new larger piece I really want
to
have the code look consistent, could you please try to mimic my style a bit
more.
(This saves me from rewriting the things which look different ;-) )
_______________________________________________________
Reply to this item at:
<http://gna.org/patch/?1054>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Wesnoth-bugs mailing list
[email protected]
https://mail.gna.org/listinfo/wesnoth-bugs
