Refactor Browser

[BigRoy]

Changelog Description

• Separate legacy loader from new browser
• Show hero versions in browser
• Allow to switch versions in the row directly by double clicking version cell (like old loader)
• Add explicit working view defaults
• Add an interface contract so that custom addons can add new columns/filers, e.g. Site Sync.
• Add columns behavior now matches more with latest frontend
• Filter bar has been tweaked to match more with latest frontend
• Optimize queries for group by where possible
• Add informational overlays when nothing is selected.
• Add "In Scene" column functionality to show which versions are already loaded
• Add "Go to current context" button when in a DCC (when you have a context)
• Allow to hide/show columns + reorder columns
• Set AYON_USE_LEGACY_LOADER=1 env var to use the legacy loader (we may want to reverse this at first)
• Refactor 'Review' terminology to 'Browser'

Additional info

This won't be an easy one to review because well, it's massive. And it still requires a lot of cleanup - but I've focused on just throwing things together to get the UI to a functional state with admittedly spending a small amount of time only on cleanup.

It'd be great to review it by:

• Testing it
• Then do some glancing over the code and ask questions about things that are entirely stupid.
• Only once we're getting to a state that we're all somewhat happy I think we should sit down and do a thorough review of everything.

Also, I'm sure the tests are shit ;) I've not read them at all and I let AI do that along the side just to give it some wheels.

And, I'm so sorry!

TODO

• Filter bar should not extend the UI size
• Add Columns menu has issues with the focus (doesn’t auto-expand)
• Group By:
  • Improve querying: probably not query groups separately, just query the versions and group them UI side.
  • Show empty groups (does not filter out the empty groups?)
• Fix invalid query being done on selecting a group (for the side panel)
• Toggle between old or new browser via setting in core
  • Allow an env var to override the setting
• Add "My Tasks" filtering to the slicer?
• Loader inside a DCC is not respecting Library attribute and shows all projects by default
• When Loader/Browser is launched for the very first time, it is empty (no folders). Closing it and running it again will populate the data, or selecting a different project and going back will refresh the data
• LoaderController + BrowserController should not both exist in the browser separated between BrowserController & BrowserWidgetController (QObject for signals) for now; needs more @iLLiCiTiT review.
• Change the columns interface around so that addon doesn't need to expose a method that requires Qt - avoid the Qt requirement + maybe change it around for something else to 'register something with core', so there some core.x.y.register_column_handler() Removed the public interface API for now
• Add Count on group by on table view
• views list UI cosmetics
• Test site sync integration with custom columns
• Add a means to group by 'productGroup' Possibly better with a server-side migration script that moves it to a regular attribute.
• Add a hardcore limit of never displaying more than e.g. 1000 versions with a little notice to recommend the user to perform tighter filtering to avoid tons of paginated queries? Skipping for now because we're paginating when we can.
• Test Power pack-less views access groups?
  • Perhaps add additional license check for power-pack?
• Fix Tasks "name" filter disappearing on folder switch due to tasks widget bottom left
• Ensure all relevant toggles, like e.g. "My Tasks" and "Group By" are stored within the Views, and when resetting the working view to "default" that it also resets both of these.

Testing notes:

1. Cry for a bit that you've been assigned reviewer here.
2. Test it functionally
3. Glance over code and ask questions, point out obvious things.

Only then after this PR has gone through initial tests, then we should go over thorough code review.

[moonyuet]

Suggested change

	len(order) != len(options_by_text)
	or set(order) != set(options_by_text)
	set(order) != set(options_by_text)

[BigRoy]

This was actually added due to copilot describing there's an edge case if we don't. Admittedly I have a hard time finding those Copilot comments now. I'll need to look that up again 🗡️

[moonyuet]

maybe we can use variable to store this or use if...elif..else store it But it is already many if..elif..elif...elif..else in this PR.

icon_name =  (
	      "check"
	      if check_style == "checkmark"
	      else "check_box"
	      if option.checked
	      else "check_box_outline_blank"
)

or

if check_style == "checkmark":
    icon_name = "check"
elif option.checked:
    icon_name = "check_box"
else:
    icon_name = "check_box_outline_blank"

[moonyuet]

@BigRoy Guess we should come up with some clean up with this function to make it more readable?

[BigRoy]

At some point - yes, but not a priority honestly - more important that it works and that it's fast. Might do that once we get to the thorough code cleanup here though. Needs the features/bugs resolved first :D

[iLLiCiTiT]

Shouldn't that be named ._icon_color_on?

[iLLiCiTiT]

Suggested change

	if dropdown_variant is None:
	self._dropdown = ButtonMenuDropdown(
	self, container_margin=dropdown_margin
	)
	else:
	self._dropdown = ButtonMenuDropdown(
	self,
	frame_variant=dropdown_variant,
	container_variant=dropdown_variant,
	container_margin=dropdown_margin,
	)
	kwargs = {}
	if dropdown_variant is not None:
	kwargs = dict(
	frame_variant=dropdown_variant,
	container_variant=dropdown_variant,
	)
	self._dropdown = ButtonMenuDropdown(
	self,
	container_margin=dropdown_margin,
	**kwargs
	)

[iLLiCiTiT]

Suggested change

	local_pos = self.mapFromGlobal(QtGui.QCursor.pos())
	local_pos = self.mapFromGlobal(QtGui.QCursor.pos())
	if isinstance(local_pos, QtCore.QPointF):
	local_pos = local_pos.toPoint()

[iLLiCiTiT]

Suggested change

	if isinstance(model, QSortFilterProxyModel):
	source = model.sourceModel()
	if isinstance(source, PaginatedTableModel):
	source.fetch_more(model.mapToSource(index))
	return
	while isinstance(model, QSortFilterProxyModel):
	model = model.sourceModel()

[iLLiCiTiT]

Can I use AYCardView with custom model? If yes, then why it's using Qt.ItemDataRole.UserRole to get UI related data?

That is potential danger for cusom models using UserRole for different purposes. Also how do we make sure that the data are actually stored there?

I didn't dig enough to find out what child_count, group_percentage and group_metrics are comming from and what are representing, but this doesn't look good unless the model is strictly defined.

[iLLiCiTiT]

I don't think this should be publicly available.

[iLLiCiTiT]

Why to change this to _fetch_more instead of fetchMore defined by Qt's QAbstractItemModel?

[iLLiCiTiT]

Suggested change

	for widget in self.take_items():
	widget.deleteLater()
	def take_items(self) -> list[QWidget]:
	"""Remove and return items without deleting their widgets."""
	widgets = [widget for widget, _match_fn in self._items]
	for widget in widgets:
	widget.hide()
	self._items_layout.removeWidget(widget)
	self._items.clear()
	self._items_layout.clear()
	while self._items_layout.count():
	item = self._items_layout.takeAt(0)
	del item
	return widgets
	self._items.clear()
	while self._items_layout.count():
	item = self._items_layout.takeAt(0)
	widget = item.widget()
	if widget is not None:
	widget.setVisible(False)
	widget.deleteLater()

I think this was already suggested.

[iLLiCiTiT]

Suggested change

	local = self._frame.mapFromGlobal(QtGui.QCursor.pos())
	local = self._frame.mapFromGlobal(QtGui.QCursor.pos())
	if isinstance(local, QtCore.QPointF):
	local = local.toPoint()

[iLLiCiTiT]

Suggested change

	return False # never consume the event
	# never consume the event
	return False

[iLLiCiTiT]

No reason to store the value.

Considering what it does, I'd rather have helper method that would do that instead of achieve it using args.

[iLLiCiTiT]

Multiply is faster then divide

Suggested change

	radius = min(radius, draw_rect.width() / 2.0, draw_rect.height() / 2.0)
	radius = min(radius, draw_rect.width() * 0.5, draw_rect.height() * 0.5)

[iLLiCiTiT]

This could just return the color? The compute_color_for_contrast takes the tuple and converts it to QColor.

[iLLiCiTiT]

Outdated comment

[iLLiCiTiT]

Why was use_context added?

[iLLiCiTiT]

Suggested change

	if use_legacy is None:
	from ayon_core.pipeline import get_current_project_name
	from ayon_core.settings import (
	get_project_settings,
	get_studio_settings,
	)
	project_name = get_current_project_name()
	if project_name:
	settings = get_project_settings(project_name)
	else:
	settings = get_studio_settings()
	use_legacy = settings["core"]["tools"]["loader"].get(
	"use_legacy_loader", False
	)
	return use_legacy
	if use_legacy is not None:
	return use_legacy
	from ayon_core.pipeline import get_current_project_name
	from ayon_core.settings import (
	get_project_settings,
	get_studio_settings,
	)
	project_name = get_current_project_name()
	if project_name:
	settings = get_project_settings(project_name)
	else:
	settings = get_studio_settings()
	return settings["core"]["tools"]["loader"].get(
	"use_legacy_loader", False
	)

[iLLiCiTiT]

Does it make sense to allow change of the variant on project combobox?

[iLLiCiTiT]

What is this for? The comments describes "column" the variable is name "keys" and AYTableHeader does store it but does not use it.