Skip to content
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

Settings - Automated Versions Listing #2498

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

Malouche-git
Copy link
Contributor

Close issue #2390

More details on what i add in (#2390 (comment))

Tell me if it's ok to remove the _addinfiles_cboxes dictionary and use the SupportedRevitVersions_CB object list directly in both _setup_addinfiles and update_addinfiles functions.
Or should I leave it as it is to make the least changes possible?

@Malouche-git Malouche-git marked this pull request as ready for review January 13, 2025 14:09
Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay... This looks promising, I ask you to go that extra mile and implement the next steps already instead of leaving todo comments 😉

@Malouche-git
Copy link
Contributor Author

Hello,
I made some changes to address and respond to the feedback.
I remove the _addinfiles_cboxes dictionary and use the supported_revit_versions_CB object list directly in both _setup_addinfiles and update_addinfiles functions.

I also made a little change in the _make_product_name function for better version name display.
return ' | {} | {}({}) {}'.format(...)
image

Thank's for feedback

Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some personal style preferences comments, feel free to ignore them (unless @jmcouffin wants to enforce them)

Deleted Revit {} to avoid passing the version number (not necessarry)
Deleted Revit {} to avoid passing the version number (not necessarry)
Deleted Revit {} to avoid passing the version number (not necessarry)
@Malouche-git
Copy link
Contributor Author

I change th logic order to avoid If / else condition and used continue in the for loop.

for checkbox  in self.supported_revit_versions_CB:
    #checkbox.IsEnabled and checkbox.IsChecked are False by default
    if checkbox.Version not in installed_revits:
        checkbox.Content += self.get_locale_string("RevitAttachment.NotInstalled") #Change local string to avoid passing the version! already in checkbox class
         continue

Doing that, i manage to use only one time the _make_product_name function so i deleted it and insert the line directly in the script :

product = installed_revits[checkbox.Version]
checkbox.Content += ' | {} | {}({}) '.format(product.Name,product.BuildNumber,product.BuildTarget)

i used += to add some note, eg :

checkbox.Content += self.get_locale_string("RevitAttachment.NotAttached")

I change the local String "RevitAttachment.NotInstalled" in all 3 dictionnary file because Revit {} format is already in the RevitVersionCB class
It uniformize the RevitAttachment local String value

@jmcouffin
Copy link
Contributor

Ouroboro situation.
Someone tested it?

I tried, local build. But I think it reads the data located in the %appdata% whereas my local build is sitting somewhere else, so... impossible to test.
And it does not read my dev clones attachments.

@Malouche-git
Copy link
Contributor Author

That seems to work for me.
I don't have a version installed for "ALL Users".
However everything else works

I'm using this branch on my actual version of pyrevit

Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last round 😉

Malouche-git and others added 3 commits January 28, 2025 14:10
…tings.smartbutton/script.py

Co-authored-by: Andrea Ghensi <[email protected]>
…tings.smartbutton/script.py

Co-authored-by: Andrea Ghensi <[email protected]>
…tings.smartbutton/script.py

Co-authored-by: Andrea Ghensi <[email protected]>
@Malouche-git
Copy link
Contributor Author

i approve all the change !
but don't know what is pipenv run black command and where to ru it ?
is it on github ? github dev ? or in my editor ?

@sanzoghenzo
Copy link
Contributor

sanzoghenzo commented Jan 30, 2025

i approve all the change ! but don't know what is pipenv run black command and where to ru it ? is it on github ? github dev ? or in my editor ?

sorry for the delay, as stated in the developer guide you have to install pipenv on a python 3.10 environment and initialize the pipenv environment, then you can run the script

@jmcouffin
Copy link
Contributor

I am about to push the release button and would like to include this one in the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants