Wireshark-bugs: [Wireshark-bugs] [Bug 6057] Lua API: add support to temporary color filters (10
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6057
Tony Trinh <tony19@xxxxxxxxx> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |tony19@xxxxxxxxx
--- Comment #1 from Tony Trinh <tony19@xxxxxxxxx> 2011-06-24 20:52:14 EDT ---
Very cool feature! +1
Some friendly peer-review comments (in no particular order):
1. Your patch has several lines of empty diffs (whitespace/newline changes?).
2. "set_tmp_filter" should probably be renamed to distinguish it from display
filters and capture filters. The Lua API currently has "set_filter" and
"apply_filter" (not very good names for the same reason), which actually refer
to display filters only. You can probably do away with "tmp", since your
comments (which will be automatically generated into HTML documentation if the
patch is approved) already indicate that it sets the color of the *current*
packet (implying the color filter takes effect only temporarily). I suggest
"set_color_filter" -- unambiguously concise.
3. There's a minor bug in an error message of set_tmp_filter():
+ if (!filter_str) {
+ WSLUA_ARG_ERROR(set_filter,TEXT,"Must be a string");
+ }
should be:
+ if (!filter_str) {
+ WSLUA_ARG_ERROR(set_XXXXX_filter,TEXT,"Must be a string");
+ }
(where "XXXXX" is driven by whatever the function name turns out to be)
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
