-
-
Notifications
You must be signed in to change notification settings - Fork 71
Add disposable preload setting to advanced settings #425
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
Conversation
8a9c8b1
to
9369545
Compare
c639372
to
315926b
Compare
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.
One black thing - while I do not love it, apparently we are to be using it, so could you put in an option to revert to default 88 characters per line? Marek approved it, and it really cuts down on black being absolutely ridiculous. I do it in this PR: https://github.com/QubesOS/qubes-desktop-linux-manager/pull/258/files#diff-037ea159eb0a7cb0ac23b851e66bee30fb838ee8d0d99fa331a1ba65283d37f7
qubesmanager/settings.py
Outdated
vm_memory = getattr(self.vm, "memory", None) | ||
vm_maxmem = getattr(self.vm, "maxmem", None) | ||
vm_preload_dispvm = int( | ||
self.vm.features.get("preload-dispvm-max", 0) or 0 |
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'm a little bit confused about this, why is there or 0
here?
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.
The , 0
is for when there is no value, the or 0
is for when the value is empty ''
. I need to use the , 0
to not fail the .get()
if there is no value to retrieve.
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 checked again, the get(..., 0)
is not necessary if there is or 0
. It also doesn't fail the get()
method without it. The or 0
covers more cases.
ui/settingsdlg.ui
Outdated
<x>0</x> | ||
<y>0</y> | ||
<width>836</width> | ||
<width>858</width> |
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 think this is accidental?
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 revert this and other settings.
<locale language="English" country="UnitedStates"/> | ||
</property> | ||
<property name="currentIndex"> | ||
<number>0</number> |
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.
also accidental?
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 on purpose, see comment below.
<widget class="QLabel" name="type_label"> | ||
<property name="font"> | ||
<font> | ||
<weight>50</weight> |
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.
why this and other font changes?
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 on purpose, changed automatically by Qt Designer 6.4.2
on Debian, maybe I need to switch to Fedora to get the 6.9.0
... https://packages.fedoraproject.org/pkgs/qt6-qttools/qt6-designer/.
qubesmanager/settings.py
Outdated
QtCore.QRegularExpression( | ||
"[a-zA-Z0-9_-]*", | ||
QtCore.QRegularExpression.PatternOption.CaseInsensitiveOption, | ||
# fmt: off |
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.
why turning off formatting in such an awkward place (the middle of function declaration)? I do not like black myself, but I don't think this is a good approach to deal with its... opinions
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 used black -l80
, with -l88
, this is possibly unnecessary..
qubesmanager/settings.py
Outdated
|
||
vm_memory = getattr(self.vm, "memory", None) | ||
vm_maxmem = getattr(self.vm, "maxmem", None) | ||
vm_preload_dispvm = int( |
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 think the preload-disposables changes should all be in one commit (and the qrexec and black stuff in their own commits)
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, that is just for easier review and reverting if some tests show regression. By the end, it will be squashed.
qubesmanager/settings.py
Outdated
else: | ||
vm_preload_dispvm = 0 | ||
self.preload_dispvm.setMinimum(0) | ||
self.preload_dispvm.setMaximum(9999) |
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.
That does not seem like a sensible maximum... (Also, max and min can be set in the ui file; easiest way to edit it is with designer-qt)
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.
Also set on the UI. I don't know a sensible maximum for this, the user can go crazy but the system won't preload if there is not enough memory. Is 50
a reasonable amount? I don't want to discourage users with a lot of memory in the future to not use the UI and use the terminal.
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.
50 sounds like a reasonable amount, yeah
ui/settingsdlg.ui
Outdated
<item row="6" column="0"> | ||
<widget class="QLabel" name="preload_dispvm_label"> | ||
<property name="text"> | ||
<string>Preload disposables:</string> |
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 think this needs an explaining tooltip that clarifies what this setting does, exactly.
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.
Done.
I though Qubes project was using |
942f71a
to
ab68aa1
Compare
Now I used |
ab68aa1
to
b580cc1
Compare
4087322
to
2680b07
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
+ Coverage 69.08% 69.19% +0.11%
==========================================
Files 17 17
Lines 3891 3909 +18
==========================================
+ Hits 2688 2705 +17
- Misses 1203 1204 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a555607
to
207745b
Compare
PipelineRetryFailed |
qubesmanager/settings.py
Outdated
if is_linux and self.init_mem.value() * 10 < self.max_mem_size.value(): | ||
self.warn_too_much_mem_label.setVisible(True) | ||
|
||
def check_disp_changes(self): |
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 does this function actually do?
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.
Nothing, outdated, removed.
207745b
to
051b7f6
Compare
44147ca
to
e866884
Compare
Failing test is unrelated to this PR and related to qubesimgconverter. |
e866884
to
16f7bd0
Compare
PipelineRetryFailed |
For: QubesOS/qubes-issues#1512