-
Notifications
You must be signed in to change notification settings - Fork 200
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
refresh host monitor while start #671
refresh host monitor while start #671
Conversation
issue AcademySoftwareFoundation#641 Using os module pulled the version number from VERSION.in file. Default value is set to 'UNKNOWN'.
issue AcademySoftwareFoundation#16 call updateRequest method when the object of class HostMonitorDockWidget is initialized.
@gregdenton @bcipriano sir I think this would trigger refresh. But this is not configurable, please guide me on how to make this configurable. |
Sure thing. I think the mechanism you will want to use here is the Some relevant spots in the code: In effect this allows us to distribute a default settings file ( So I think we want to use this mechanism and add a new setting which controls whether Hosts auto-refresh on open. |
Added check box for setting up triggered refresh on start
Call updateRequest method if TriggeredRefresh is enabled in setting.
@bcipriano @gregdenton Sir |
Added check box (by default checked) to trigger a refresh on launch. This will make easier for users to toggle the settings. |
@srbhss Could you post a screenshot of the GUI changes you've made here? |
|
@srbhss Thanks for the thoughtful reply. I think that in almost all cases, a user will want "Auto refresh on launch" and "Auto refreshing while running" to have the same value. And for the rare cases when they don't, it will be easy to update the default setting or just toggle the checkbox. So let's use just one dropbox, the existing "Auto-refresh" one. |
@bcipriano Sir |
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.
Thanks srbhss. This change LGTM.
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 is looking mostly good!
I think there's a problem with the QtGui.qApp.settings.value()
calls. In my environment at least, when AutoRefreshMonitorHost
is set in the settings file, value()
returns the value as a string. So if we saved 0
as the value, it is returned as '0'
(a string) which is then interpreted as True
by bool()
.
I think you need to int()
the value first, before passing it to bool()
.
(I wish there was a better way to represent boolean values but I didn't find one after some quick testing.)
@bcipriano sir |
cuegui/cuegui/HostMonitor.py
Outdated
self.__viewHostsSetup() # For view_hosts signal | ||
self.__viewHostsSetup() # For view_hosts signal | ||
|
||
if bool(QtGui.qApp.settings.value("AutoRefreshMonitorHost", 1)): # For refresh on launch |
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 believe this line needs to convert to int
as well.
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.
Yes, you are right. My mistake.
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.
correction made.
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 believe bool(int(x)))
call is redundant, is there any benefit over simply allowing Python to handle the truthiness check?
for example:
if int(QtGui.qApp.settings.value("AutoRefreshMonitorHost", 1)): ...
has the same effect as
if bool(int(QtGui.qApp.settings.value("AutoRefreshMonitorHost", 1))): ...
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.
@gregdenton sir, you are right. I have used bool() to make the mechanism more clear to understand. It doesn't have any significance as far as working of code is concerned. It could be removed if it is looking bit too heavy-handed.
IMO it is good to store and use bool value for conditional statements.
For example bool value stored below:
self.enableRefresh = bool(int(QtGui.qApp.settings.value("AutoRefreshMonitorHost", 1)))
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.
Yeah, I'm a bit torn on this one. The bool()
is technically not necessary but it does make it more explicit, which is especially useful in a class-wide variable like in @srbhss's example.
I think I would lean towards keeping the bool cast.
@bcipriano please check the final update and merge. |
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.
It's very close and the code is working, there's just one comment to resolve regarding the value that gets stored in the ini
file.
cuegui/cuegui/HostMonitor.py
Outdated
@@ -256,6 +260,7 @@ def __refreshToggleCheckBoxSetup(self, layout): | |||
|
|||
def __refreshToggleCheckBoxHandle(self, state): | |||
self.hostMonitorTree.enableRefresh = bool(state) | |||
QtGui.qApp.settings.setValue("AutoRefreshMonitorHost", int(state)) |
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.
When I tested this change, when the checkbox is checked state
here is equal to 2
, causing me to end up with a config file like this:
[General]
AutoRefreshMonitorHost=2
AutoRefreshMonitorProc=2
LastNotice=1587770087
<cut>
While the code works, I think it's best to ensure only 0
or 1
end up in that settings file as it's less confusing. A 2
would seem to imply that there's more than two options when there aren't.
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.
IMO it is because our checkbox has check-values 0 for 'False', 1 for 'Null' and 2 for 'True'.
One solution is to pass bool() before int() i.e. int(bool(state))
Other is to change the check-value for states.
Using first solution will solve the problem even if I am wrong about the actual problem.
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.
@bcipriano sir, changes done. Ready for review.
cuegui/cuegui/ProcMonitor.py
Outdated
@@ -120,6 +124,7 @@ def __refreshToggleCheckBoxSetup(self, layout): | |||
|
|||
def __refreshToggleCheckBoxHandle(self, state): | |||
self.procMonitorTree.enableRefresh = bool(state) | |||
QtGui.qApp.settings.setValue("AutoRefreshMonitorProc", int(state)) |
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.
Same comment here as above, regarding the integer value of state
.
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.
Working great! Thank you!
issue #163
Call updateRequest method when the object of class HostMonitorDockWidget is constructed.