-
-
Notifications
You must be signed in to change notification settings - Fork 393
Allow for an option to close other outputs for all scripts in pyrevit settings #2793
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: develop
Are you sure you want to change the base?
Changes from 6 commits
d48b37b
ef4b916
a05cfa7
7a13621
a2c9c2b
0c3b43c
683bbc3
b1a81a2
88f13ff
c61c752
2be7743
1f188ea
b231f2d
1820cac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -376,6 +376,64 @@ public string GetFullHtml() { | |||||||||||||||
return ScriptConsoleConfigs.DOCTYPE + head.OuterHtml + ActiveDocument.Body.OuterHtml; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
private static (bool closeOthers, string closeMode) ReadCloseOtherOutputsSetting() { | ||||||||||||||||
try { | ||||||||||||||||
var appData = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData); | ||||||||||||||||
var cfg = System.IO.Path.Combine(appData, "pyRevit", "pyRevit_config.ini"); | ||||||||||||||||
if (!System.IO.File.Exists(cfg)) | ||||||||||||||||
return (false, "current_command"); | ||||||||||||||||
|
||||||||||||||||
bool inCore = false; | ||||||||||||||||
bool close = false; | ||||||||||||||||
string mode = "current_command"; | ||||||||||||||||
foreach (var raw in System.IO.File.ReadAllLines(cfg)) { | ||||||||||||||||
var line = raw.Trim(); | ||||||||||||||||
if (line.Length == 0 || line.StartsWith("#") || line.StartsWith(";")) | ||||||||||||||||
continue; | ||||||||||||||||
|
||||||||||||||||
if (line == "[core]") { inCore = true; continue; } | ||||||||||||||||
if (line.StartsWith("[") && line.EndsWith("]")) { inCore = false; continue; } | ||||||||||||||||
|
||||||||||||||||
if (!inCore) continue; | ||||||||||||||||
|
||||||||||||||||
if (line.StartsWith("close_other_outputs", StringComparison.InvariantCultureIgnoreCase)) | ||||||||||||||||
close = line.IndexOf("true", StringComparison.InvariantCultureIgnoreCase) >= 0; | ||||||||||||||||
|
if (line.StartsWith("close_other_outputs", StringComparison.InvariantCultureIgnoreCase)) | |
close = line.IndexOf("true", StringComparison.InvariantCultureIgnoreCase) >= 0; | |
if (line.StartsWith("close_other_outputs", StringComparison.InvariantCultureIgnoreCase)) { | |
var parts = line.Split(new[] { '=' }, 2); | |
if (parts.Length == 2) | |
close = parts[1].Trim().Trim('"').Equals("true", StringComparison.InvariantCultureIgnoreCase); | |
} |
Copilot uses AI. Check for mistakes.
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.
Empty catch block without logging. This makes debugging difficult if config reading fails. Consider adding logging or at least a comment explaining why exceptions are silently ignored.
catch (Exception ex) {
// Log configuration parsing error for troubleshooting
System.Diagnostics.Debug.WriteLine($"Failed to read pyRevit config: {ex.Message}");
return (false, "current_command");
}
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
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.
Empty catch block without logging. UI operations can fail for various reasons and silent failures make troubleshooting difficult.
catch (Exception ex) {
// Log UI operation error for troubleshooting
System.Diagnostics.Debug.WriteLine($"Failed to close other outputs: {ex.Message}");
}
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
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.
Empty catch block violates exception handling standards. According to the review validation checklist, empty catch blocks should log exceptions at minimum. Consider logging the exception or using more specific exception types.
catch (Exception ex) {
logger.Debug("Failed to close other output windows: " + ex.Message);
}
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
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.
It is better to put the operating modes in an enum or create a class hierarchy
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 a lot!
Here, would you add extension methods to map enum and string from Consts file ?
I'm a beginner in C# so plz don't be implicit :)
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.
after writing the review, I think it will be enough to create text constants with which to conduct comparisons
but you can still make an enum Enum.TryParse<TEnum>(string value, out TEnum result)
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,3 +38,5 @@ tooltip: | |
|
||
Zeigt die Konfigurationsdatei im Explorer an. | ||
context: zero-doc | ||
engine: | ||
clean: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,6 +157,15 @@ def _setup_core_options(self): | |
|
||
self.loadbetatools_cb.IsChecked = user_config.load_beta | ||
|
||
self.minimize_consoles_cb.IsChecked = user_config.output_close_others | ||
|
||
if user_config.output_close_mode == 'current_command': | ||
|
||
self.closewindows_current_rb.IsChecked = True | ||
self.closewindows_close_all_rb.IsChecked = False | ||
else: # 'close_all' | ||
self.closewindows_current_rb.IsChecked = False | ||
self.closewindows_close_all_rb.IsChecked = True | ||
|
||
def _setup_engines(self): | ||
"""Sets up the list of available engines.""" | ||
attachment = user_config.get_current_attachment() | ||
|
@@ -846,6 +855,12 @@ def _save_core_options(self): | |
|
||
user_config.load_beta = self.loadbetatools_cb.IsChecked | ||
|
||
user_config.output_close_others = self.minimize_consoles_cb.IsChecked | ||
if self.closewindows_current_rb.IsChecked: | ||
user_config.output_close_mode = 'current_command' | ||
else: | ||
user_config.output_close_mode = 'close_all' | ||
|
||
def _save_engines(self): | ||
# set active cpython engine | ||
engine_cfg = self.cpythonEngines.SelectedItem | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,21 @@ | |
from pyrevit.coreutils import configparser | ||
from pyrevit.coreutils import logger | ||
from pyrevit.versionmgr import upgrade | ||
# pylint: disable=C0103,C0413,W0703 | ||
MohamedAsli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import os | ||
import os.path as op | ||
|
||
from pyrevit import EXEC_PARAMS, HOME_DIR, HOST_APP | ||
from pyrevit import EXTENSIONS_DEFAULT_DIR, THIRDPARTY_EXTENSIONS_DEFAULT_DIR | ||
from pyrevit import PYREVIT_ALLUSER_APP_DIR, PYREVIT_APP_DIR | ||
from pyrevit import PyRevitException | ||
from pyrevit import coreutils | ||
from pyrevit.compat import winreg as wr | ||
from pyrevit.coreutils import appdata | ||
from pyrevit.coreutils import configparser | ||
from pyrevit.coreutils import logger | ||
from pyrevit.labs import PyRevit | ||
from pyrevit.versionmgr import upgrade | ||
|
||
MohamedAsli marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
DEFAULT_CSV_SEPARATOR = ',' | ||
|
||
|
@@ -302,6 +316,36 @@ def load_beta(self, state): | |
value=state | ||
) | ||
|
||
@property | ||
def output_close_others(self): | ||
"""Whether to close other output windows.""" | ||
return self.core.get_option( | ||
'close_other_outputs', | ||
|
||
default_value=False, | ||
) | ||
|
||
@output_close_others.setter | ||
def output_close_others(self, state): | ||
self.core.set_option( | ||
'close_other_outputs', | ||
|
||
value=state | ||
) | ||
|
||
@property | ||
def output_close_mode(self): | ||
"""Output window closing mode: 'current_command' or 'close_all'.""" | ||
return self.core.get_option( | ||
'close_mode', | ||
|
||
default_value='current_command', | ||
) | ||
|
||
@output_close_mode.setter | ||
def output_close_mode(self, mode): | ||
self.core.set_option( | ||
'close_mode', | ||
|
||
value=mode | ||
) | ||
|
||
@property | ||
def cpython_engine_version(self): | ||
"""CPython engine version to use.""" | ||
|
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 recommend using the configuration API, which is already in the pyRevit library. It is also worth adding these settings to the command line utility :)