Thanks for the PR!

It looks like the CI is failing for the Qt4 builds (which are also out python 2 pipelines). I'm on mobile so I can't easily look into the specific failures until later. Is there a portion of your PR that wouldn't be python 2 or Qt4 compatible?

Just took a peek at the CI. The error is Qt4 related and we have run into it a number of times. There is another PR being held back for the same fault (until Qt4 support is deprecated which won't be long now).

I'll leave it up to you if you want to try and identify the Qt4 issue and remedy it, vs sitting tight until we deprecated Qt4 and we can evaluate merging then.

The failure for Qt4 seems to be because tighter tolerances are in place for Qt4
https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/tests/image_testing.py#L260
Or rather, Qt5 is succeeding because of looser tolerances for it.

I do make use of the connected mode feature and the following snippet shows a regression with this pull request.

import pyqtgraph
import numpy as np

app = pyqtgraph.mkQApp()

plotwgt = pyqtgraph.PlotWidget()
curve = pyqtgraph.PlotCurveItem()
plotwgt.addItem(curve)

x = [10, 20, 20, 10, 10, np.nan, 30, 40, 40, 30, 30]
y = [10, 10, 20, 20, 10, np.nan, 10, 10, 20, 20, 10]
curve.setData(x=x, y=y, connect='finite')
        
plotwgt.show()
app.exec_()

Running on Python 3.7.6 64-bits PyQt 5.9.6 Windows 10

Without this pull request:

With this pull request:

@pijyoi thanks for pointing out the tolerance issue, I remember that being discussed in the past now that you bring it up; thanks also for bringing up the regression this PR addresses.

I'm good with loosening the tolerance on the Qt4 based tests a bit if that's the only issue.

Thanks @j9ac9k for the quick review and @pijyoi for the test. Indeed, I did not use the other connection types except the default one. I fixed two cases with connection types being 'pairs' and 'finite'. I am not sure whether we should have some unittest them.

Thanks for the update @zhujun98 the Qt4 related tests pass now 🎊

I'm not sure what a unit test for this would look like... given our really awful poor test coverage, additional tests would be much appreciated. If you have something in mind, please feel free to add it to the PR.

Given the low level nature of this PR, I'm going to ask some of the other maintainers to take a look as well.

This looks good to me. I don't actually notice any change in behavior (it seems all segments are drawn even when using cStart=1), but definitely this makes the code more "correct". Thanks @zhujun98!

For users directly using the connect=ndarray codepath, there would be a change in user-facing API.
In the snippet below, users would have to make an update to their connect array.

One possibility is to retain the old user-facing API and convert it under-the-hood to the new syntax.

import pyqtgraph
import numpy as np

app = pyqtgraph.mkQApp()

plotwgt = pyqtgraph.PlotWidget()
curve = pyqtgraph.PlotCurveItem()
plotwgt.addItem(curve)

def connect_api_change(c):
    c = np.roll(c, 1)
    c[0] = 0
    return c

x = [10, 20, 20, 10, 10, 30, 40, 40, 30, 30]
y = [10, 10, 20, 20, 10, 10, 10, 20, 20, 10]
# c = np.array([1, 1, 1, 1, 0, 1, 1, 1, 1, 0])        # old syntax
# c = connect_api_change(c)
c = np.array([0, 1, 1, 1, 1, 0, 1, 1, 1, 1])        # new syntax
curve.setData(x=x, y=y, connect=c)

plotwgt.show()
app.exec_()