Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[ Feature ] Multi Tab - License Tab#246

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
PhoneDroid wants to merge14 commits intoFreeCAD:dev
base:dev
Choose a base branch
Loading
fromPhoneDroid:Feature-Tabs

Conversation

@PhoneDroid
Copy link
Member

Inspired by the work donehere

I decided to go with a minimal
approach for my own implementation.

  • Adds multi tabs to addon overviews.
  • Adds aLicense tab

Help wanted, if someone with Python async knowledge
could make this not block the UI, that's be just great ..


Implementation Notes

I decided to have mapping of the tabs to their tab indices
to not have to hard code them and keep them in sync.

The license text is currently just dumped in the
tab, for now I just wanted to get it to work.

elifself.url.lower().endswith(".html"):
self.readme_data_type=ReadmeDataType.Html

withopen(self.url,"r")asfd:

Check warning

Code scanning / CodeQL

File is not always closed Warning

File may not be closed if an exception is raised.
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Not my fault, seize!

Copy link
Member

@chenneschennes left a comment

Choose a reason for hiding this comment

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

A few minor comments, below. Also your new code includes blank lines between nearly every line of code, so I don't know if your IDE doesn't like something or what.

Comment on lines +153 to +162
defloadLicense(self,license:License,text:QtWidgets.QTextEdit):

url=utils.construct_git_url(self.addon,license.file)

manager=NetworkManager.AM_NETWORK_MANAGER

index=manager.submit_unmonitored_get(url)

self.license_requests[index]= {"license":license,"text":text}

Copy link
Member

Choose a reason for hiding this comment

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

That's a lot of whitespace here, for no benefit that I can see.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Same reason as using mutliline or names other than
x ,y ,z for variable names, makes it easier to read?
Group together what belongs together..

You have auto-formatting rules, if you don't
like it, shouldn't you have configured that?

Copy link
Member

Choose a reason for hiding this comment

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

The rule with using Black is, you don't configure Black 😁 -- we break that by setting the line length, but otherwise there aren't any settings. Black will let you put individual blank lines in as you see fit, which is good because it's basically impossible to check that programmatically. I'm just questioning these blank lines: there's nothing to "group", you have four individual lines. So the only grouping is that they are all part of theloadLicense method (which PEP8 says should be calledload_license).

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Well yeah, each line does something different, if I had something like x = y() and then a bunch of indexing into x like x.z = '' I'd put them together.

I have a hard time reading blocks of text so I space things 0 , 1 , 2 lines apart depending on the context.

I can smush them all together if you'd like.

self.tabs_widget=QtWidgets.QTabWidget(self)

self.tabs[TabView.Readme]=self.tabs_widget.addTab(
self.readme_browser,translate("AddonsInstaller","README")
Copy link
Member

Choose a reason for hiding this comment

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

@FreeCAD/design-working-group what's our canonical capitalization of "README"?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Doesn't seem like we will get any more comments any time soon, ..
I adjusted it toOverview +License.

@chennes
Copy link
Member

I'll give this a refactoring pass today or tomorrow -- we shouldn't use aWidgetReadmeBrowser if ultimately we need a whole list ofQTextEdit in the tab. Licenses are strange, and probably need their own class and controller.

PhoneDroid reacted with eyes emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chenneschenneschennes left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PhoneDroid@chennes

[8]ページ先頭

©2009-2025 Movatter.jp