refresh host monitor while start #671

srbhss · 2020-03-29T13:46:19Z

issue #163

Call updateRequest method when the object of class HostMonitorDockWidget is constructed.

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.

srbhss · 2020-03-30T09:26:37Z

@gregdenton @bcipriano sir I think this would trigger refresh. But this is not configurable, please guide me on how to make this configurable.

bcipriano · 2020-03-31T02:20:04Z

Sure thing. I think the mechanism you will want to use here is the QSettings system we already have in place.

Some relevant spots in the code:

Where the settings .ini file is read in.
As an example, how we store the LogEditor setting.

In effect this allows us to distribute a default settings file (cuetopia.ini) that studio sysadmins can override by distributing their own custom version.

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.

srbhss · 2020-04-03T13:03:50Z

@bcipriano @gregdenton Sir
This will resolve the issue. Please check if everything is looking fine.

srbhss · 2020-04-05T10:25:01Z

Added check box (by default checked) to trigger a refresh on launch. This will make easier for users to toggle the settings.
Will it be a good feature ?
@bcipriano Sir

bcipriano · 2020-04-06T23:07:37Z

@srbhss Could you post a screenshot of the GUI changes you've made here?

srbhss · 2020-04-07T16:53:24Z

@bcipriano Sir
We can configure the 'refreshing on start' from this 'triggered-refresh' check box. Please comment if this is a good option or It would be better with something else.

bcipriano · 2020-04-10T20:23:05Z

@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.

srbhss · 2020-04-11T17:25:33Z

@bcipriano Sir
Please check this again. I have set auto refresh 'ON' by default and integrated refresh on launch with it.
Did same with both "Host Monitor" and "Proc Monitor".

gregdenton

Thanks srbhss. This change LGTM.

bcipriano

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.)

cuegui/cuegui/HostMonitor.py

srbhss · 2020-04-16T16:49:18Z

@bcipriano sir
I think now it will work fine, used int() to convert string value to int before passing to bool()

bcipriano · 2020-04-20T22:23:50Z

cuegui/cuegui/HostMonitor.py

- self.__viewHostsSetup() # For view_hosts signal
+ self.__viewHostsSetup() # For view_hosts signal
+
+ if bool(QtGui.qApp.settings.value("AutoRefreshMonitorHost", 1)): # For refresh on launch


I believe this line needs to convert to int as well.

Yes, you are right. My mistake.

correction made.

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))): ...

@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)))

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.

srbhss · 2020-04-21T09:34:16Z

@bcipriano please check the final update and merge.

bcipriano

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.

bcipriano · 2020-04-24T23:18:15Z

cuegui/cuegui/HostMonitor.py

@@ -256,6 +260,7 @@ def __refreshToggleCheckBoxSetup(self, layout):

 def __refreshToggleCheckBoxHandle(self, state):
 self.hostMonitorTree.enableRefresh = bool(state)
+ QtGui.qApp.settings.setValue("AutoRefreshMonitorHost", int(state))


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.

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.

@bcipriano sir, changes done. Ready for review.

bcipriano · 2020-04-24T23:18:58Z

cuegui/cuegui/ProcMonitor.py

@@ -120,6 +124,7 @@ def __refreshToggleCheckBoxSetup(self, layout):

 def __refreshToggleCheckBoxHandle(self, state):
 self.procMonitorTree.enableRefresh = bool(state)
+ QtGui.qApp.settings.setValue("AutoRefreshMonitorProc", int(state))


Same comment here as above, regarding the integer value of state.

bcipriano

Working great! Thank you!

srbhss added 10 commits March 19, 2020 15:24

Version number pulled from version.in file to constant.py

526fd23

issue AcademySoftwareFoundation#641 Using os module pulled the version number from VERSION.in file. Default value is set to 'UNKNOWN'.

Update cuerqd.py

2a3755a

Update cuerqd.py

5f4d401

Update cuerqd.py

97175a3

Update Constants.py

e21f167

Update Constants.py

dc20e08

Update Constants.py

bd0fcda

Update Constants.py

c0b8aea

Update Constants.py

158f826

updateRrequest method on initializing

a3f7504

issue AcademySoftwareFoundation#16 call updateRequest method when the object of class HostMonitorDockWidget is initialized.

srbhss marked this pull request as ready for review March 30, 2020 09:22

srbhss requested review from bcipriano, gregdenton, jrray, larsbijl and smith1511 as code owners March 30, 2020 09:22

srbhss closed this Mar 30, 2020

srbhss reopened this Mar 30, 2020

srbhss added 3 commits April 3, 2020 17:05

check Box triggered refresh option

7d25469

Added check box for setting up triggered refresh on start

Enabled triggered refresh

66c520b

Call updateRequest method if TriggeredRefresh is enabled in setting.

import QtGui in MonitorHostsPlugin.py

a0db258

Updated doc

7d07c6b

srbhss added 2 commits April 7, 2020 22:14

bug fixing

b2726f7

Update MonitorHostsPlugin.py

f32ad24

srbhss added 9 commits April 11, 2020 22:24

Update HostMonitorTree.py

7f2b965

Update HostMonitor.py

7f3f822

Update MonitorHostsPlugin.py

caa6f9a

Update HostMonitorTree.py

b82ca44

Update MonitorHostsPlugin.py

43d15c2

Update HostMonitorTree.py

c0afc70

Update HostMonitor.py

c125cfe

Update ProcMonitorTree.py

d4187e0

Update ProcMonitor.py

f0f9356

gregdenton approved these changes Apr 12, 2020

View reviewed changes

bcipriano requested changes Apr 15, 2020

View reviewed changes

cuegui/cuegui/HostMonitor.py Outdated Show resolved Hide resolved

srbhss added 4 commits April 16, 2020 21:59

settings updated

491e660

Update settings

d0d89f1

resolved bool value issue

b8e3a86

bool value issue resolved

c6d0ad5

srbhss added 2 commits April 16, 2020 23:15

Update HostMonitorTree.py

3caa032

Update ProcMonitorTree.py

73a19ef

bcipriano requested changes Apr 20, 2020

View reviewed changes

srbhss added 2 commits April 21, 2020 14:58

Update HostMonitor.py

8361b13

Update ProcMonitor.py

6a78fa2

bcipriano requested changes Apr 24, 2020

View reviewed changes

srbhss added 2 commits April 27, 2020 13:45

Update HostMonitor.py

7e4c854

Update ProcMonitor.py

586f102

bcipriano approved these changes Apr 30, 2020

View reviewed changes

bcipriano merged commit 4f815a6 into AcademySoftwareFoundation:master Apr 30, 2020

bcipriano mentioned this pull request Feb 19, 2021

Launching Monitor Hosts should trigger a refresh #163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refresh host monitor while start #671

refresh host monitor while start #671

srbhss commented Mar 29, 2020

srbhss commented Mar 30, 2020

bcipriano commented Mar 31, 2020

srbhss commented Apr 3, 2020 •

edited

srbhss commented Apr 5, 2020

bcipriano commented Apr 6, 2020

srbhss commented Apr 7, 2020 •

edited

bcipriano commented Apr 10, 2020

srbhss commented Apr 11, 2020

gregdenton left a comment

bcipriano left a comment

srbhss commented Apr 16, 2020 •

edited

bcipriano Apr 20, 2020

srbhss Apr 21, 2020

srbhss Apr 21, 2020

gregdenton Apr 22, 2020

srbhss Apr 22, 2020

bcipriano Apr 24, 2020

srbhss commented Apr 21, 2020

bcipriano left a comment

bcipriano Apr 24, 2020

srbhss Apr 26, 2020

srbhss Apr 27, 2020

bcipriano Apr 24, 2020

bcipriano left a comment

refresh host monitor while start #671

refresh host monitor while start #671

Conversation

srbhss commented Mar 29, 2020

srbhss commented Mar 30, 2020

bcipriano commented Mar 31, 2020

srbhss commented Apr 3, 2020 • edited

srbhss commented Apr 5, 2020

bcipriano commented Apr 6, 2020

srbhss commented Apr 7, 2020 • edited

bcipriano commented Apr 10, 2020

srbhss commented Apr 11, 2020

gregdenton left a comment

Choose a reason for hiding this comment

bcipriano left a comment

Choose a reason for hiding this comment

srbhss commented Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srbhss commented Apr 21, 2020

bcipriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcipriano left a comment

Choose a reason for hiding this comment

srbhss commented Apr 3, 2020 •

edited

srbhss commented Apr 7, 2020 •

edited

srbhss commented Apr 16, 2020 •

edited