Merge lp:~oppifjellet/cable/message-history into lp:cable
- message-history
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 128 | ||||
Proposed branch: | lp:~oppifjellet/cable/message-history | ||||
Merge into: | lp:cable | ||||
Diff against target: |
405 lines (+258/-10) 7 files modified
CMakeLists.txt (+22/-1) src/Utils/RRStringList.vala (+77/-0) src/Widgets/Room.vala (+31/-9) tests/TestCase.vala (+58/-0) tests/TestExample.vala (+22/-0) tests/TestMain.vala (+7/-0) tests/TestMessageHistory.vala (+41/-0) |
||||
To merge this branch: | bzr merge lp:~oppifjellet/cable/message-history | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Eduard Gotwig | Pending | ||
Review via email: mp+182453@code.launchpad.net |
Commit message
Implemented message history
Description of the change
This branch fixes this bug https:/
Every channel now keeps an history of sent messages and commands that are navigable using arrow keys ( Up&Down )
For performance reasons the history capacity for each channel is 25 items. Old items are replaced with round robin logic.
Eduard Gotwig (gotwig) wrote : | # |
- 126. By Daniele "OpenNingia" Simonetti
-
improved history navigation behavior
Daniele "OpenNingia" Simonetti (oppifjellet) wrote : | # |
Revision 126 should fix the annoying behavior.
You're welcome, thank you for Cable.
982c80311320c1b (alexander-wilms) wrote : | # |
It doesn't work as expected for me.
When I type
1 <Enter>
2 <Enter>
3 <Enter>
and then browse through history with the <up> key, the following messages appear in this order: 2, 1. When I then press the <down> key, those messages appear: 1, 2, 3
If I _instead_ (not after browing them with <up> and <down>) press the <down> key, 3 appears in the text box.
Daniele "OpenNingia" Simonetti (oppifjellet) wrote : | # |
I admit it's a strange behavior. If it were you what's the best behavior?
Il giorno 30/ago/2013 00:57, "Alexander Wilms" <email address hidden>
ha scritto:
> It doesn't work as expected for me.
> When I type
> 1 <Enter>
> 2 <Enter>
> 3 <Enter>
>
> and then browse through history with the <up> key, the following messages
> appear in this order: 2, 1. When I then press the <down> key, those
> messages appear: 1, 2, 3
>
> If I _instead_ (not after browing them with <up> and <down>) press the
> <down> key, 3 appears in the text box.
> --
> https:/
> You are the owner of lp:~oppifjellet/cable/message-history.
>
Daniele "OpenNingia" Simonetti (oppifjellet) wrote : | # |
So I checked the implementation:
pressing <down><up> with the editbox empty ( or after sending a message ) is a shortcut to get the last typed message )
instead when navigating <up>/<down> will return the next and the previous message, with one exception:
. the history doesn't save duplicates
so if you write
1
2
3
2
the last "2" is ignored.
982c80311320c1b (alexander-wilms) wrote : | # |
I think the most intuitive implementation (and IMHO the most simple one) would be this:
After typing:
1
2
3
2
Pressing <down> doesn't do anything. <Up> shows the last history entry (2) and every press of <up> shows the previous message. <Down> returns the next message.
Duplicates should not be skipped, since users don't know the algorithm behind it and at least I would expect it to list the messages in the order in which I pyed them.
- 127. By Daniele "OpenNingia" Simonetti
-
permit duplicates
Daniele "OpenNingia" Simonetti (oppifjellet) wrote : | # |
Ok so, this revision implements your design. Duplicates are permitted, history default size has been increased to 50, and hitting <down> after typing an entry does nothing.
982c80311320c1b (alexander-wilms) wrote : | # |
Great :)
- 128. By Daniele "OpenNingia" Simonetti
-
add unit test using GLib.Test
Eduard Gotwig (gotwig) wrote : | # |
You shouldnt propose merge requests, which are based on other merge requests. Each merge request should be clean applied with it changes on trunk.
However, I am going to look today at the other merge request of you, which included this one.
Cheers,
Preview Diff
1 | === modified file 'CMakeLists.txt' |
2 | --- CMakeLists.txt 2013-08-06 15:32:12 +0000 |
3 | +++ CMakeLists.txt 2013-09-03 20:19:34 +0000 |
4 | @@ -55,6 +55,7 @@ |
5 | src/Utils/EmbeddedAlert.vala |
6 | src/Utils/Identity.vala |
7 | src/Utils/Network.vala |
8 | + src/Utils/RRStringList.vala |
9 | |
10 | src/Irc/Maki.vala |
11 | src/Irc/Irc.vala |
12 | @@ -90,7 +91,7 @@ |
13 | src/Dialogs/Preferences.vala |
14 | src/Dialogs/ManageIdentity.vala |
15 | src/Dialogs/ManageNetwork.vala |
16 | - |
17 | + |
18 | src/Tests/Tests.vala |
19 | src/Tests/Utils.vala |
20 | |
21 | @@ -109,9 +110,29 @@ |
22 | --verbose |
23 | ) |
24 | |
25 | +vala_precompile(VALA_C_TESTS |
26 | + tests/TestCase.vala |
27 | + tests/TestMain.vala |
28 | + tests/TestExample.vala |
29 | + tests/TestMessageHistory.vala |
30 | + src/Utils/RRStringList.vala |
31 | +PACKAGES |
32 | + gtk+-3.0 |
33 | + granite |
34 | + libnotify |
35 | + unity |
36 | +OPTIONS |
37 | + --target-glib=2.32 |
38 | + --vapidir=${CMAKE_CURRENT_SOURCE_DIR}/vapi/ |
39 | + --enable-experimental |
40 | + --fatal-warnings |
41 | + --verbose |
42 | +) |
43 | + |
44 | add_subdirectory (po) |
45 | add_subdirectory (data) |
46 | |
47 | add_executable(cable ${VALA_C}) |
48 | +add_executable(cable_tests ${VALA_C_TESTS}) |
49 | |
50 | install(TARGETS cable RUNTIME DESTINATION bin) |
51 | |
52 | === added file 'src/Utils/RRStringList.vala' |
53 | --- src/Utils/RRStringList.vala 1970-01-01 00:00:00 +0000 |
54 | +++ src/Utils/RRStringList.vala 2013-09-03 20:19:34 +0000 |
55 | @@ -0,0 +1,77 @@ |
56 | +/*** |
57 | + Copyright (C) 2013 Cable Developers |
58 | + |
59 | + This program or library is free software; you can redistribute it |
60 | + and/or modify it under the terms of the GNU Lesser General Public |
61 | + License as published by the Free Software Foundation; either |
62 | + version 3 of the License, or (at your option) any later version. |
63 | + |
64 | + This library is distributed in the hope that it will be useful, |
65 | + but WITHOUT ANY WARRANTY; without even the implied warranty of |
66 | + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU |
67 | + Lesser General Public License for more details. |
68 | + |
69 | + You should have received a copy of the GNU Lesser General |
70 | + Public License along with this library; if not, write to the |
71 | + Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, |
72 | + Boston, MA 02110-1301 USA. |
73 | +***/ |
74 | + |
75 | + |
76 | +public class Cable.Collections.RRStringList { |
77 | + |
78 | + internal int? current_; |
79 | + internal Gee.List <string> data; |
80 | + |
81 | + public int capacity { get; set; } |
82 | + public int? current_index { get { return current_; } } |
83 | + |
84 | + public RRStringList.with_capacity (int capacity_value) |
85 | + requires (capacity_value > 0) { |
86 | + capacity = capacity_value; |
87 | + data = new Gee.ArrayList <string>(); |
88 | + } |
89 | + |
90 | + public RRStringList () { |
91 | + this.with_capacity(50); // default capacity |
92 | + } |
93 | + |
94 | + public void add(string value) { |
95 | + if ( data.size >= this.capacity ) |
96 | + data.remove_at( 0 ); |
97 | + data.add(value); |
98 | + current_ = data.size/*-1*/; |
99 | + } |
100 | + |
101 | + public bool contains(string value) { |
102 | + return data.contains(value); |
103 | + } |
104 | + |
105 | + public string current() { |
106 | + if ( current_ == null ) return ""; |
107 | + if ( current_ >= data.size ) current_ = data.size-1; |
108 | + if ( current_ < 0 ) current_ = 0; |
109 | + return data[current_]; |
110 | + } |
111 | + |
112 | + public bool previous() { |
113 | + if ( current_ == null ) return false; |
114 | + if ( current_ <= 0 ) return false; |
115 | + current_ = current_ - 1; |
116 | + return true; |
117 | + } |
118 | + |
119 | + public bool next() { |
120 | + if ( current_ == null ) return false; |
121 | + if ( current_+1 >= data.size ) return false; |
122 | + current_ = current_ + 1; |
123 | + return true; |
124 | + } |
125 | + |
126 | + public bool set_current_value(string value) { |
127 | + if ( !(value in data) ) return false; |
128 | + current_ = data.index_of(value)+1; |
129 | + return true; |
130 | + } |
131 | + |
132 | +} |
133 | |
134 | === modified file 'src/Widgets/Room.vala' |
135 | --- src/Widgets/Room.vala 2013-08-22 18:40:53 +0000 |
136 | +++ src/Widgets/Room.vala 2013-09-03 20:19:34 +0000 |
137 | @@ -51,6 +51,8 @@ |
138 | internal Gtk.Entry text_entry; |
139 | Utils.AutoScrolled left_scrolled; |
140 | |
141 | + internal Cable.Collections.RRStringList history_set; |
142 | + |
143 | Gtk.Box user_box; |
144 | |
145 | public Widgets.Topic topic_bar; |
146 | @@ -62,17 +64,21 @@ |
147 | public signal void rejoin (); |
148 | |
149 | public string channel { get; set; } |
150 | - |
151 | + |
152 | public string nick { |
153 | get { return pseudo_label.label; } |
154 | set { pseudo_label.label = value; } |
155 | } |
156 | - |
157 | + |
158 | public string entry { |
159 | get { return text_entry.text; } |
160 | set { text_entry.text = value; } |
161 | } |
162 | |
163 | + public Cable.Collections.RRStringList history { |
164 | + get { return history_set; } |
165 | + } |
166 | + |
167 | State _state; |
168 | public State state { |
169 | get { return _state; } |
170 | @@ -104,7 +110,7 @@ |
171 | show_all (); |
172 | } |
173 | } |
174 | - |
175 | + |
176 | private Gee.Set <string> nicks { |
177 | owned get { |
178 | var _nicks = new Gee.HashSet <string> (); |
179 | @@ -119,6 +125,8 @@ |
180 | public Room (string channel, string nick="") { |
181 | this.channel = channel; |
182 | |
183 | + history_set = new Cable.Collections.RRStringList(); |
184 | + |
185 | init_layout (); |
186 | connect_callbacks (); |
187 | } |
188 | @@ -227,7 +235,7 @@ |
189 | return true; |
190 | } else { |
191 | text_entry.buffer.delete_text (text_entry.cursor_position - word.length, word.length); |
192 | - |
193 | + |
194 | if( text_entry.text != "" ){ |
195 | text_entry.insert_at_cursor (completion[0] + " "); |
196 | } else { |
197 | @@ -236,12 +244,26 @@ |
198 | return true; |
199 | } |
200 | } |
201 | + else if ( ev.keyval == Gdk.Key.Up ) { |
202 | + if ( history.previous() ) |
203 | + text_entry.text = history.current(); |
204 | + return true; |
205 | + } |
206 | + else if ( ev.keyval == Gdk.Key.Down ) { |
207 | + if ( history.next() ) |
208 | + text_entry.text = history.current(); |
209 | + return true; |
210 | + } |
211 | return false; |
212 | }); |
213 | - |
214 | + |
215 | text_entry.activate.connect (() => { |
216 | - if (text_entry.text != "") |
217 | + if (text_entry.text != "") { |
218 | + |
219 | + // permit duplicates |
220 | + history.add(text_entry.text); |
221 | send_message (text_entry.text); // signal |
222 | + } |
223 | }); |
224 | |
225 | action_join.activate.connect (() => { |
226 | @@ -281,7 +303,7 @@ |
227 | requires (!has_user (new_name)) |
228 | ensures (!has_user (old_name)) |
229 | ensures (has_user (new_name)) { |
230 | - |
231 | + |
232 | if (old_name == null) { |
233 | add_user (new_name, type, true); |
234 | return; |
235 | @@ -300,7 +322,7 @@ |
236 | |
237 | var user = new User (name); |
238 | user.selectable = false; |
239 | - |
240 | + |
241 | switch (type) { |
242 | case UserType.OPERATOR: |
243 | operators.add (user); |
244 | @@ -324,7 +346,7 @@ |
245 | public bool remove_user (string name, string message, bool silent = false) |
246 | requires (has_user (name)) |
247 | ensures (!has_user (name)) { |
248 | - |
249 | + |
250 | Granite.Widgets.SourceList.ExpandableItem[] lists = {voiced, operators, regular}; |
251 | foreach (var list in lists) { |
252 | foreach (var child in list.children) { |
253 | |
254 | === added directory 'tests' |
255 | === added file 'tests/TestCase.vala' |
256 | --- tests/TestCase.vala 1970-01-01 00:00:00 +0000 |
257 | +++ tests/TestCase.vala 2013-09-03 20:19:34 +0000 |
258 | @@ -0,0 +1,58 @@ |
259 | +public abstract class Gee.TestCase : Object { |
260 | + |
261 | + private GLib.TestSuite suite; |
262 | + private Adaptor[] adaptors = new Adaptor[0]; |
263 | + |
264 | + public delegate void TestMethod (); |
265 | + |
266 | + public TestCase (string name) { |
267 | + this.suite = new GLib.TestSuite (name); |
268 | + } |
269 | + |
270 | + public void add_test (string name, owned TestMethod test) { |
271 | + var adaptor = new Adaptor (name, (owned)test, this); |
272 | + this.adaptors += adaptor; |
273 | + |
274 | + this.suite.add (new GLib.TestCase (adaptor.name, |
275 | + adaptor.set_up, |
276 | + adaptor.run, |
277 | + adaptor.tear_down )); |
278 | + } |
279 | + |
280 | + public virtual void set_up () { |
281 | + } |
282 | + |
283 | + public virtual void tear_down () { |
284 | + } |
285 | + |
286 | + public GLib.TestSuite get_suite () { |
287 | + return this.suite; |
288 | + } |
289 | + |
290 | + private class Adaptor { |
291 | + |
292 | + public string name { get; private set; } |
293 | + private TestMethod test; |
294 | + private TestCase test_case; |
295 | + |
296 | + public Adaptor (string name, |
297 | + owned TestMethod test, |
298 | + TestCase test_case) { |
299 | + this.name = name; |
300 | + this.test = (owned)test; |
301 | + this.test_case = test_case; |
302 | + } |
303 | + |
304 | + public void set_up (void* fixture) { |
305 | + this.test_case.set_up (); |
306 | + } |
307 | + |
308 | + public void run (void* fixture) { |
309 | + this.test (); |
310 | + } |
311 | + |
312 | + public void tear_down (void* fixture) { |
313 | + this.test_case.tear_down (); |
314 | + } |
315 | + } |
316 | +} |
317 | \ No newline at end of file |
318 | |
319 | === added file 'tests/TestExample.vala' |
320 | --- tests/TestExample.vala 1970-01-01 00:00:00 +0000 |
321 | +++ tests/TestExample.vala 2013-09-03 20:19:34 +0000 |
322 | @@ -0,0 +1,22 @@ |
323 | +class TestExample : Gee.TestCase { |
324 | + |
325 | + public TestExample() { |
326 | + // assign a name for this class |
327 | + base("TestExample"); |
328 | + // add test methods |
329 | + add_test("test_example", test_example); |
330 | + } |
331 | + |
332 | + public override void set_up () { |
333 | + // setup your test |
334 | + } |
335 | + |
336 | + public void test_example() { |
337 | + // add your expressions |
338 | + // assert(expression); |
339 | + } |
340 | + |
341 | + public override void tear_down () { |
342 | + // tear down your test |
343 | + } |
344 | +} |
345 | \ No newline at end of file |
346 | |
347 | === added file 'tests/TestMain.vala' |
348 | --- tests/TestMain.vala 1970-01-01 00:00:00 +0000 |
349 | +++ tests/TestMain.vala 2013-09-03 20:19:34 +0000 |
350 | @@ -0,0 +1,7 @@ |
351 | +public static int main(string[] args) { |
352 | + Test.init (ref args); |
353 | + // add any of your test cases here |
354 | + TestSuite.get_root().add_suite(new TestExample().get_suite()); |
355 | + TestSuite.get_root().add_suite(new TestMessageHistory().get_suite()); |
356 | + return Test.run (); |
357 | +} |
358 | \ No newline at end of file |
359 | |
360 | === added file 'tests/TestMessageHistory.vala' |
361 | --- tests/TestMessageHistory.vala 1970-01-01 00:00:00 +0000 |
362 | +++ tests/TestMessageHistory.vala 2013-09-03 20:19:34 +0000 |
363 | @@ -0,0 +1,41 @@ |
364 | +class TestMessageHistory : Gee.TestCase { |
365 | + |
366 | + public TestMessageHistory() { |
367 | + // assign a name for this class |
368 | + base("TestMessageHistory"); |
369 | + // add test methods |
370 | + add_test("[History] Navigate history", test_navigate); |
371 | + add_test("[History] Limited capacity", test_capacity); |
372 | + } |
373 | + |
374 | + public override void set_up () { |
375 | + // setup your test |
376 | + } |
377 | + |
378 | + public void test_navigate() { |
379 | + var history = new Cable.Collections.RRStringList(); |
380 | + history.add("one"); |
381 | + history.add("two"); |
382 | + history.add("three"); |
383 | + assert(history.current() == "three"); |
384 | + assert(history.next() == false); |
385 | + assert(history.current() == "three"); |
386 | + assert(history.previous() == true); |
387 | + assert(history.current() == "two"); |
388 | + } |
389 | + |
390 | + public void test_capacity() { |
391 | + var history = new Cable.Collections.RRStringList.with_capacity(2); |
392 | + history.add("one"); |
393 | + history.add("two"); |
394 | + history.add("three"); |
395 | + assert(history.current() == "three"); |
396 | + assert(history.previous() == true); |
397 | + assert(history.previous() == false); |
398 | + assert(history.current() == "two"); |
399 | + } |
400 | + |
401 | + public override void tear_down () { |
402 | + // tear down your test |
403 | + } |
404 | +} |
405 | \ No newline at end of file |
sometimes I have to double press up or down, for it to work. Can this be fixed?
Thanks for your work.