Merge lp:~sylvain-pineau/checkbox/fix-1477423 into lp:checkbox

Proposed by Sylvain Pineau
Status: Merged
Approved by: Zygmunt Krynicki
Approved revision: 3919
Merged at revision: 3918
Proposed branch: lp:~sylvain-pineau/checkbox/fix-1477423
Merge into: lp:checkbox
Diff against target: 49 lines (+19/-2)
1 file modified
checkbox-touch/components/CommandOutputPage.qml (+19/-2)
To merge this branch: bzr merge lp:~sylvain-pineau/checkbox/fix-1477423
Reviewer Review Type Date Requested Status
Zygmunt Krynicki (community) Approve
Sylvain Pineau Needs Resubmitting
Review via email: mp+266006@code.launchpad.net

Description of the change

Fixes the linked bug

To post a comment you must log in.
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

Ha, so the issue was caused by the text modification events? How many events did you see here? What was the granularity?

review: Needs Information
Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

I've just updated the bug report. Let's say that 1000 packages worked, but 2500 not.

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

I've replied to the bug as well. I'd rather do it correctly rather
than quickly so let's work on this (perhaps tomorrow as it's getting
late) to understand where the fault lies and then address it at that
time.

For now, I'd like to posppone this from landing.

On Mon, Jul 27, 2015 at 6:50 PM, Sylvain Pineau
<email address hidden> wrote:
> I've just updated the bug report. Let's say that 1000 packages worked, but 2500 not.
> --
> https://code.launchpad.net/~sylvain-pineau/checkbox/fix-1477423/+merge/266006
> You are reviewing the proposed merge of lp:~sylvain-pineau/checkbox/fix-1477423 into lp:checkbox.

Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

Updated version to update the textarea text with the I/O delta, not the full logs.

review: Needs Resubmitting
3919. By Sylvain Pineau

checkbox-touch:CommandOutputPage: Display the IO buffer only on request and every 300ms

To avoid being flooded by asynchronous calls and request to append text to the
TextArea, all IO events are buffered in a new string property.

The TextArea text is only updated when the CommandOutputPage is visible and
only every 300ms (and only the delta is appended to the text property)

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

As discussed on irc, modifying the text property is inefficient. We should use the append method of the TextEdit component hidden inside the TextArea

http://doc.qt.io/qt-5/qml-qtquick-textedit.html#text-prop

Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

I replaced the TextArea with TextEdit to get access to the append method but unfortunately it hangs the same way. So the Timer/buffer are so far the only viable solution.

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

I kind of don't like one thing (but I suspect qt compensates for that). The timer runs regardless of any updates being needed or not. I would prefer to only start the timer (in one-shot more) when we know there is something to do.

Oh, and let's file those upstream bugs, this should not be needed.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'checkbox-touch/components/CommandOutputPage.qml'
2--- checkbox-touch/components/CommandOutputPage.qml 2015-06-19 13:24:04 +0000
3+++ checkbox-touch/components/CommandOutputPage.qml 2015-07-28 08:03:55 +0000
4@@ -32,12 +32,14 @@
5 id: commandOutputPage
6
7 objectName: "commandOutputPage"
8+ property string bufferedIO;
9
10 function addText(text) {
11- textArea.text += text;
12+ bufferedIO += text
13 }
14
15 function clear() {
16+ bufferedIO = "";
17 textArea.text = "";
18 }
19
20@@ -54,6 +56,16 @@
21 ]
22 }
23
24+ Timer {
25+ id: timer
26+ interval: 300
27+ running: false
28+ repeat: true
29+ onTriggered: {
30+ textArea.text += bufferedIO
31+ bufferedIO = ""
32+ }
33+ }
34
35 ColumnLayout {
36 spacing: units.gu(1)
37@@ -88,6 +100,11 @@
38 }
39 onVisibleChanged: {
40 // Pop-over should be displayed only when page becomes visible (in practice - when it's pushed to the pageStack)
41- if (visible == true) fadeOutDelay.start();
42+ if (visible == true) {
43+ timer.running = true;
44+ }
45+ else {
46+ timer.running = false;
47+ }
48 }
49 }

Subscribers

People subscribed via source and target branches