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

Implement #127 and #96 #129

Merged
merged 28 commits into from
Nov 18, 2020

Conversation

davidlatwe
Copy link
Collaborator

@davidlatwe davidlatwe commented Nov 11, 2020

What's Changed

Update: see #129 (comment)

Internal Refactor

  • PackagesModel and ApplicationModel has been merged into one ResolvedPackagesModel.
  • Two proxy model were added to adopt the above merge : ApplicationProxyModel and PackagesProxyModel.
  • Controller is no longer required for resetting package model.

Discussion

After package version list has been limited to only show in the range that profile requested, the latest flag is not reflecting real latest anymore.

But do we need to know whether a package is latest or not ? I do imaging that sometime we indeed want to know that info, but should not be often. So I would propose to implement that query action in a menu, and only scan all versions when one needed.

Secondly, do we still need the "Set to earliest" and "Set to latest" action ?

action snapshot

image

I am more lean to respect the request what profile have made, and not allowing change denpency version freely.

What do you think ?

PackagesModel and ApplicationModel are merged into one
ResolvedPackagesModel with ApplicationProxyModel
and PackagesProxyModel added.

Also, model doesn't need controller now.
@davidlatwe
Copy link
Collaborator Author

Ah, one more thing, the "Beta".
Since the version string already contains "beta", do we still need package model to parse that info ?

snapshot

image

@davidlatwe
Copy link
Collaborator Author

Was looking at the development journal #LatestVersion, and it was enough for me to keep the Beta column. :P

The reason for me to brought that up was only try to make model data lesser.

About the Latest, I think maybe we could make a background thread to collect that info after model rested.

@mottosso
Copy link
Owner

But do we need to know whether a package is latest or not ?

I think so? Especially as you develop I would think? And especially as someone else updates a package you aren't working with directly.

But how do you mean "real" latest? What else is it showing?

Secondly, do we still need the "Set to earliest" and "Set to latest" action ?

We need to make it an option at least, something that can be configured. Potentially something visible to developers, but not to artists.

@davidlatwe
Copy link
Collaborator Author

But how do you mean "real" latest? What else is it showing?

The implementation of limiting version list is to find package versions with fixed range, which leads to only showing the latest one of that range.

Shouldn't be hard to solve, and with the input you gave, I think I have a rough UI/UX plan for overriding versions now. 🤔

First would be to implement a background thread to fetch a full version list for each package in context, after profile resolved with all applications. Once collected, all packages can override version freely if user ticked the checkbox e.g. "Unlock version range".

@davidlatwe
Copy link
Collaborator Author

Maybe, one extra function would be, auto bump or down other dependencies version if current version changing package leads to a failed resolve.

allzpark/control.py Outdated Show resolved Hide resolved
@RafaelVillar
Copy link

Hello @davidlatwe ! The work you have been doing is fantastic!

In regards to the "real latest" question. I think the latest of "range" is an okay assumption to make, no? I understand it may not be the literal latest of but if it was an artist is allowed then that should be ok. I think this feature originally sprang from a discussion of convenience to quickly return something back to default or if you are developer testing packages, you would be incrementing.

@mottosso
Copy link
Owner

(For context, Allzpark was initially developed for Rafael's studio. He'll understand the pre-existing requirements well)

Add a preference setting that will show all package
versions.
Profile requested application version range will still
be respected, but all versions of each dependency package
will be shown.
Allow admin to block preference from user with config
@davidlatwe
Copy link
Collaborator Author

davidlatwe commented Nov 12, 2020

Thanks @RafaelVillar and @mottosso !

I understand it may not be the literal latest of but if it was an artist is allowed then that should be ok.

Agree :)

So I end up with adding "Show All Versions" in preference.

When that enabled, dependency package will be able to select all versions from repository, except applications that has fixed range set by profile. This should reduce the chance of selecting version that can't be resolved with profile.

I also make packages that has only one version available not triggering version editing delegate widget, and set bold font for package that has many. This should make user much easier to know which is changeable.

And since "Show All Versions" is mostly for developer, I also implemented a way to lock preference with default value from allzparkconfig.py.

Lastly, I found that graph button will be disabled even package is not missing but only conflicted, which is a bug from #124, so I fixed it together in this PR.

Screenshot

lock

allzpark/control.py Outdated Show resolved Hide resolved
@davidlatwe
Copy link
Collaborator Author

davidlatwe commented Nov 13, 2020

I found packages' resolve order become incorrect in "Packages" dock widget after the change that collecting all packages and versions in one place with merged model.

So I went back and after reviewing my changes a few times, I decide to revert the PackagesModel and ApplicationMode merge, mainly for two reasons.

One is to fix the package resolve ordering of course, and the other is, I started to worry that not only reset time will become longer, but also it will be hard to implement feature like "not loading resolved packages when 'Packages' dock page is not visible/active" (I remember this has been logged in some where but can't find it).

I think this is a good revert.

Oh and, this PR also fixes #87.

To be more test friendly.

Widgets (qargparse widgets in this case) were defined
as class attributes (not in __init__), so it won't be
garbage collected after main window closed, and signals
are still connected. And somehow they still able to
trigger the model which is created in next test session
to do model reset. So how many tests has ran, how many
reset signal will be emitted, to the same model, hence
the test can easily got segfault (especially Linux).
@davidlatwe
Copy link
Collaborator Author

After adding version editing test cases, I found changing preference "Show All Versions" in test will get segfault easily.

So I had a look and found it was because the preference widget (qargparse widgets) were defined as Preferences dock widget's class attribute (not defined in __init__), which makes them not able to be garbage collected when tearing down the test.

Somehow, those preference widgets that survived from previous tests still able to touch models that are created in current running test, and how many test has ran, how many model rest may called, when changing preference that calls ctrl.reset, e.g. "Show All Versions", hence segfault.

However, although after commit feed9c3 resolved the problem in new test case (tested with my Windows and WSL Ubuntu), test running on Azure DevOp still have a little chance getting segfault. The journey hasn't end.

@davidlatwe
Copy link
Collaborator Author

davidlatwe commented Nov 15, 2020

Ok, I think this is stable now, here's what we have now :

  • App package version can be changed in main view (Change Application Version #96)
  • App's version list has been limited to profile request range (Limiting package versions to profile request range #127)
  • Right-clicking on any column in Packages view can bring up version editor (Right-click in packages tab #87)
  • By default, only app that has multi-versions can change/patch version in main Apps view or in Packages page
  • If preference "Show All Versions" enabled, all package versions will be listed, except profile and app package
  • App selection won't lost (jump to first app in view) after patching
  • App version won't be editable if versions has been flattened (allzpark.com#multiple-application-versions)
  • Preference setting can be blocked from allzparkconfig.protected_preferences to prevent touching danger stuff
  • No TableModel needs Controller object to get packages now (tech-debt paid)
  • A few tests added for this PR

And here's the changes in action :

(Since dependency package in allzparkdemo only core_pipeline has multiple versions, so only that package can be edit when "Show All Version" enabled)

latest

Ready to merge now :)

@mottosso
Copy link
Owner

Great work, David. :)

@davidlatwe
Copy link
Collaborator Author

Merging this later today :)

@davidlatwe davidlatwe mentioned this pull request Nov 18, 2020
@davidlatwe davidlatwe merged commit 64a7762 into mottosso:master Nov 18, 2020
@davidlatwe davidlatwe deleted the feature/implement-#127-#96 branch November 18, 2020 10:56
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