vitalyf (Vitaly Fanaskov)
User

Projects

User does not belong to any projects.

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Saturday

  • Clear sailing ahead.

User Details

User Since
Mar 4 2019, 9:15 AM (264 w, 3 d)
Availability
Available

Recent Activity

Aug 28 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this.

Hi @vkrause, Any luck here?

If I'm not mistaken this patch still raises the required Qt version from 5.5 to at least 5.9, right? If so that still needs to be addressed (as in: restore Qt 5.5 compatibility) before this can be integrated. I'm sorry if that wasn't clear.

Aug 28 2019, 8:04 AM

Aug 27 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this.

Aug 27 2019, 1:30 PM
vitalyf requested review of D23491: Add CMake options to make build process more flexible.
Aug 27 2019, 1:27 PM

Apr 15 2019

vitalyf requested review of D20575: Add static build support..
Apr 15 2019, 2:16 PM

Apr 12 2019

vitalyf updated the diff for D20160: Extract functionality for submitting collected data.

DefaultDataSubmitter is set by default. I also changed "#pragma once" to include guards.

Apr 12 2019, 8:37 AM

Apr 11 2019

vitalyf updated the diff for D20160: Extract functionality for submitting collected data.

I've fixed the test and also removed 5.13 introduced code.

Apr 11 2019, 9:56 AM

Apr 10 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

Conceptually and structurally I think this is getting there, but we need to address the minimum Qt requirement :) Right now even 5.12 isn't enough due to the Q_DISABLE_COPY_MOVE it seems, so I can't easily test this here.

Apr 10 2019, 12:36 PM

Apr 9 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

Regarding redirects handling, I suggest just use mechanism introduced in Qt 5.6. That one I already implemented, but this of course will be moved to the second class which is more specific.

5.6 or 5.9? As mentioned before with GammaRay using this an needing 5.5 support this is a problem. If we are talking about 5.6 I could possibly just update the snapshot GammaRay uses to before this change and hope we wont need another update before GammaRay doesn't support 5.5 anymore. For 5.9 this is more of an issue though.

Apr 9 2019, 10:10 AM
vitalyf updated the diff for D20160: Extract functionality for submitting collected data.
Apr 9 2019, 10:09 AM

Apr 4 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

Sharing common bits of network code makes perfect sense of course. Not knowing your backend interface makes it hard to suggest specifics though. What is the main difference to the current interface, ie. which parts do you actually need to be able to control?

Only requests, I would say. The new class should look like this:

enum class SubmissionOption { ... }

class BaseDataSubmitter : public AbstractDataSubmitter
{
public:
     void submit(const QByteArray &data) const final;

    SubmissionOptions options() const;
    void setOptions(KUserFeedback::SubmissionOptions options);

protected:
   virtual QNetworkRequest probeRequest() const;
   virtual QNetworkRequest dataRequest() const = 0;

   // ...
};

Basically, what I propose is to split AbstractDataSubmitter I implemented before, to two classes: more abstract interface and more specific implementation to keep common and useful functionality. If a user need to implement something specific, he could take the top-level interface class AbstractDataSubmitter, if it would be enough to just slightly configure requests, he could take BaseDataSubmitter (or we can name it QtBasedDataSubmitter).

Or maybe [Abstract]RestDataSubmitter? As you said earlier, any implementation is likely based on Qt :) Having virtual methods to get the requests makes sense, and is a manageable public interface (which I assume is what this will be, as your implementation will be external, right?). I'm wondering though if we can avoid the submission options.

That contains 3 options from what I can see:

  • add mimetype and payload size header fields: if you need to customize that you could adjust it in the xRequest() methods too, right? Same as e.g. adding an API key header. I don't like these options as they well overly specific, and I don't even think our current backend cares much about those headers being present or not, ie. if you need something specific for your backend, maybe we can just set that as default?
  • enabling the probe request: that makes more sense on first sight, however the reason why this was added is that redirects on POST is apparently somewhat undefined in the HTTP standard, and quite inefficient as you send the full payload just to learn you have to do so again. And redirection support is something you most likely want in long-term deployments, to be able to adjust your infrastructure on the server over time. If you want to avoid that, how are you going to deal with this issue instead?
Apr 4 2019, 1:47 PM
vitalyf added a comment to D20160: Extract functionality for submitting collected data.

@vkrause, I'm thinking about adding one more basic class for sending data. This class will inherit the interface we discussed above, but also will provide some common utilities based on QtNetwork things (I think, it should be functionality, we decided to remove from basic class). I propose name like "BaseDataSubmitter" or "QtBasedDataSubmitter". We need this, to avoid massive code duplication.
What do you think about this idea?

Sharing common bits of network code makes perfect sense of course. Not knowing your backend interface makes it hard to suggest specifics though. What is the main difference to the current interface, ie. which parts do you actually need to be able to control?

Apr 4 2019, 1:11 PM
vitalyf added a comment to D20160: Extract functionality for submitting collected data.

@vkrause, I'm thinking about adding one more basic class for sending data. This class will inherit the interface we discussed above, but also will provide some common utilities based on QtNetwork things (I think, it should be functionality, we decided to remove from basic class). I propose name like "BaseDataSubmitter" or "QtBasedDataSubmitter". We need this, to avoid massive code duplication.
What do you think about this idea?

Apr 4 2019, 11:15 AM

Apr 2 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

So, you propose interface like this:

class AbstractDataSubmitter : public QObject
{
public:
    ~AbstractDataSubmitter() override;
Apr 2 2019, 11:58 AM
vitalyf added a comment to D20160: Extract functionality for submitting collected data.

I like the general approach of factoring out the data submission, and making that an extension vector. I am however unsure about how that interface should look like. This now exposes very fine grained switches (like setting of certain HTTP headers) and protocol internals (the probing GET before POSTing, to support redirects), which feels like we are doing this at a too low level. I can't imagine the presence of a content length header being the decisive difference in practice for example.

Shouldn't this rather be on the level of "here's a JSON document, a server URL and product id, submit it and let me know how that went"? That is, leave all the details of HTTP headers, probing requests, etc to the actual implementation? One could even argue that no QtNetwork usage should leak through that interface, even if that is probably unnecessarily extreme.

Apr 2 2019, 9:47 AM

Apr 1 2019

vitalyf added a comment to D20160: Extract functionality for submitting collected data.

Didn't have time to look into (1) and (2) yet, but the reason for (3) is compatibility all the way back to Qt 5.5, which is oldest version still supported by GammaRay.

Apr 1 2019, 11:48 AM
vitalyf requested review of D20160: Extract functionality for submitting collected data.
Apr 1 2019, 9:43 AM

Mar 19 2019

vitalyf added a comment to D19882: Make store and load methods public.

Huh? I must have misclicked, sorry, this was supposed to be approved :)

Mar 19 2019, 3:41 PM
vitalyf added a comment to D19882: Make store and load methods public.

@vkrause, What kind of changes are required?

Mar 19 2019, 2:38 PM
vitalyf requested review of D19882: Make store and load methods public.
Mar 19 2019, 1:35 PM

Mar 18 2019

vitalyf updated the diff for D19757: Add method to check whether a data source is active or not.

"Active" state is not reset in between data submissions anymore.
Also fixed unit test for this piece of functionality.

Mar 18 2019, 10:58 AM

Mar 15 2019

vitalyf updated the diff for D19757: Add method to check whether a data source is active or not.

"Active" state is stored in the settings now.
Also added unit tests covers new functionality.

Mar 15 2019, 3:46 PM
vitalyf added a comment to D19757: Add method to check whether a data source is active or not.

I had option (2) in mind initially, assuming that this doesn't hurt even for scenarios where this functionality isn't needed. It would also be consistent with how the current settings are persisted, avoiding the surprise that disabled elements get automatically activated again if you don't implement persistence manually in your application. (1) would achieve the same thing I think, so I'd be ok with that too. I just don't like option (3) :)

Mar 15 2019, 10:10 AM
vitalyf updated the diff for D19758: Data source can be obtained by ID.

Changed the method name and added unit test covers lookup functionality.

Mar 15 2019, 10:07 AM
vitalyf added a comment to D19757: Add method to check whether a data source is active or not.

Do we need to persist this setting?

Mar 15 2019, 8:53 AM
vitalyf added inline comments to D19758: Data source can be obtained by ID.
Mar 15 2019, 8:42 AM

Mar 14 2019

vitalyf requested review of D19758: Data source can be obtained by ID.
Mar 14 2019, 12:30 PM
vitalyf requested review of D19757: Add method to check whether a data source is active or not.
Mar 14 2019, 12:18 PM
vitalyf updated the diff for D19756: Add method name() to the *DataSource classes.

Slightly changed documentation format to match code around

Mar 14 2019, 10:56 AM
vitalyf requested review of D19756: Add method name() to the *DataSource classes.
Mar 14 2019, 10:53 AM
vitalyf requested review of D19754: Add build directory to the .gitignore.
Mar 14 2019, 10:18 AM

Mar 13 2019

vitalyf updated the diff for D19727: Rename method name() to id().

Some accidentally renamed parameters restored back

Mar 13 2019, 3:17 PM
vitalyf added inline comments to D19727: Rename method name() to id().
Mar 13 2019, 3:08 PM
vitalyf requested review of D19727: Rename method name() to id().
Mar 13 2019, 2:06 PM
vitalyf retitled D19725: Add a method to get a short name of a telemetry mode from Add FeedbackConfigUiController to the headers for installation to Add a method to get a short name of a telemetry mode.
Mar 13 2019, 1:35 PM
vitalyf added a comment to D19725: Add a method to get a short name of a telemetry mode.

The change makes sense, but the commit message seems unrelated :)

Mar 13 2019, 1:33 PM
vitalyf retitled D19725: Add a method to get a short name of a telemetry mode from Added FeedbackConfigUiController to the installation script to Add FeedbackConfigUiController to the headers for installation.
Mar 13 2019, 1:32 PM
vitalyf requested review of D19725: Add a method to get a short name of a telemetry mode.
Mar 13 2019, 12:50 PM
vitalyf requested review of D19724: Added FeedbackConfigUiController to the installation script.
Mar 13 2019, 12:48 PM

Mar 7 2019

vitalyf added a comment to D19505: Fix wrong include.

If you don't have commit rights yet, I can land this for you, but I'd need an email address to properly attribute it.

Mar 7 2019, 8:14 AM

Mar 6 2019

vitalyf added a comment to D19505: Fix wrong include.

I can't seem to reproduce this here, nor can the CI apparently. Also, those two header files are in different folders here...
Hm, are you using a different build system by any chance?

Mar 6 2019, 2:10 PM
vitalyf added a comment to D19505: Fix wrong include.

Does this break the build for you? Since this header is technically from a different library, the use of the angle brackets doesn't seem wrong to me...

Mar 6 2019, 8:13 AM

Mar 4 2019

vitalyf requested review of D19505: Fix wrong include.
Mar 4 2019, 9:25 AM