-
Notifications
You must be signed in to change notification settings - Fork 265
Feature/check for updates #751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Is there a use case for this? Or should it be indicated to the user to check for updates when they want? Would it be better to push a notification to the user that an update is available once it is, rather then checking every 3 months? |
A notification or a pop-up will be pushed to the user only when there is a real update. The check happens once in 3 months by default. If the user wants to change the timing from 3 months to any other value, it can be customised. If suppose the user wants to check if any updates are available, it can also be checked by using the option in help menu. |
dlt-viewer
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this submodule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this got added. Have removed it in this commit
src/mainwindow.h
Outdated
| QDltSettingsManager *settings; | ||
|
|
||
| //Update Pop Up | ||
| updateChecker *updChecker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- class names shall start with capital case
- please do not save on letters and write full words when naming vars, aka updateChecker
- comment is not correct, because besides popup update checker is the whole engine to do the work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HAve made the required changes. Thanks for the review correction
src/settingsdialog.cpp
Outdated
| QSettings settings("MyCompany", "DLTViewer"); | ||
|
|
||
| bool isCustom = settings.value("updateCheck/useCustom", false).toBool(); | ||
| int interval = settings.value("updateCheck/customMonths", 3).toInt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 is a magical constant which is used in different places, please define it clearly with a good name in a global scope
|
is there a way to disable the check completely? Ubuntu users normally stick to version existing in their repos |
src/updatechecker.cpp
Outdated
| // ----------------------------- | ||
| // Popups | ||
| // ----------------------------- | ||
| void updateChecker::showUpdatePopup(const QString ¤t, const QString &latest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
popup is usually a very poor UX for such information. Normally new version is reported somewhere in a main window as a non-disturbing message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pop-up doesnt interrupt everytime dlt viewer is opened. It pops up only when the time line reaches. If suppose its given as a undisturbing pop-up, the users who might need an update will never know that there is an update. Thats why its given in the screen itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the pop up is in the right bottom corner, there are high chances that the pop up will be ignored by the user.
The main purpose of this feature is to intimate the user that there is a latest update available in DLT Viewer.
In VS Code the pop up usually comes in the right bottom corner which is ignored by many users most of the time. The same issue will happen in DLT Viewer also. That's why I tried to implement the pop up in the center, so that the response has to be given by the user to continue working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can offer some input as I was involved in the original request. The intent is mainly to inform about major releases, which AFAIK are planned to ramp up to 3-4 per year, and will generally speaking have both important bug fixes which warrant a minor release but hopefully broader and more useful features. This was motivated because we have a lot of users who are using old(er) versions and frequently complain of problems which are solved in recent releases.
Setting the check interval to an arbitrary timespan, or disable it, I think should be offered. A pop-up once every three months when starting up should not be too intrusive but is certainly more intrusive than an icon in the bottom bar. From my PoV grabbing attention like that is desirable. I also think the latter would be mostly ignored due to its persistence. Some other applications go with some text in the bottom bar (e.g. the Zed editor prints something like "Click here to restart and update Zed", next to checkmark or exclamation icon) which I think less of than the pop-up -
the argument could be made that too much text in the bottom bar can be distracting (I certainly feel that way even about the existing "Version" string).
Anyway, broadly speaking I would not be opposed to putting this notification in the bottom bar, but if so, I would prefer the text to be quite short.
Yes. When there is a pop-up the user can give ignore as well, which will never pop up again till the timeline reaches. Its completely the user's choice to update or no. We are just intimating the user that there is an update available. |
| #endif | ||
|
|
||
| // Define default update check interval in months | ||
| static const int DEFAULT_UPDATE_CHECK_MONTHS = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the earlier comments - maybe it would be best to change the unit here to days, just in case a user wants to have frequent checks.
|
Just one request from me @Renu-Priya411 , could you squash the changes so we avoid the merge commits and make this easier to review? thx |
Changes in settings.cpp A bug fix in QSettings, for taking the user given time for automatic pop-up for Check For Updates. Signed-off by : Renu Priya Krishnamoorthy <[email protected]> Changes in mainwindow, settingsDialog and updateChecker Global declaration for QSettings. Class name change. Signed-off by : Renu Priya Krishnamoorthy <[email protected]> Export to DLT does not work for non-verbose messages Extended header was missing while exporting non-verbose messages to DLT. Signed-off by : Pavithra Anand <[email protected]> Update qdltmsg.cpp Use consistent needOfExtendedHeader logic in genMsg()
0cf6780 to
321a957
Compare
An automatic pop-up for newer version of DLT Viewer once in 3 months.
Signed-off by : Renu Priya Krishnamoorthy [email protected]