Upstream rebase

[isantop]

Upstream Appcenter has had many nice changes since the last time we synced up; this pulls in the latest updated code from AppCenter to bring these new developments into Pop_Shop.

Improvements Include:

• Improvements to asynchronous processing to improve performance and reduce UI blockages.
• Redesigned top-bar which improves use with narrow form factor windows and better highlights search functionality.
• Fixes for issues related to "Open" button for newly-installed Apps
• Reductions in changes from upstream, simplifying future maintenance.
• Status overlay to better communicate background progress

[mmstick]

I get a segmentation fault the moment it finishes fetching download sizes while loading packages for a category. I'll run it through gdb and try to find out why a gobject is being unref'd once too many times.

[mmstick]

The root of the problem is a thread safety violation between the main thread and the flatpak backend.

[mmstick]

There's still an occasional crash on the home page that happens at random involving a pango layout being NULL. Looking around there's a lot of thread safety violations so it might be caused by one of them.

Essentially, the flatpak and packagekit backends are creating components on background threads at the same time the homepage, category views, and app info views in the main thread are accessing and mutating the same data.

Problem is that's difficult to determine what data is being shared across threads. Doesn't seem to be any locks, and locking will certainly block the UI as before.

[mmstick]

Seems to be fixed now. The homepage now asynchronously waits for the packagekit and flatpak threads to finish before it starts adding packages to the carousel. And the Ubuntu drivers backend also asynchronously waits for the packagekit thread to finish before it does its work.

[isantop]

@mmstick I'm now getting a segfault any time I try and open the application:

io.elementary.appcenter

(io.elementary.appcenter:25102): Gtk-WARNING **: 08:36:04.006: Theme parsing error: application.css:42:19: The :insensitive pseudo-class is deprecated. Use :disabled instead.
[1]    25102 segmentation fault (core dumped)  io.elementary.appcenter

[mmstick]

They happen at random. There's still several instances of shared memory in a thread race

[mmstick]

Every search seems to cut off the first few characters that were typed

[isantop]

They happen at random. There's still several instances of shared memory in a thread race

They seem to happen to me every time I try and start it. It's definitely not random.

[isantop]

It appears that as of caf11a86051b4c98e3ff0a0200f72fd4f5e517ed I'm getting a segfault every time the app opens.

[ids1024]

I don't see a segfault, but valgrind does indicate a couple memory errors: https://gist.github.com/ids1024/49f4a23aa55c4969b95228e861b13642

(Pop shop takes a long time to start in valgrind. AddressSanitizer should be faster if you can get it to build with it.)

Edit: Okay, address sanitizer can be used with meson -Db_sanitize=address build.

[isantop]

In my testing I'm able to reproduce this reliably when starting the application from scratch; it segfaults on every run when opening while not running in daemon mode. If the XDG Autostart service opens Pop Shop in daemon mode, it opens correctly and can be used as normal.

The issue only happens as of caf11a8 and is fine on 6df40f4

[isantop]

I can definitively reproduce this using the following procedure:

1. Use killall io.elementary.appcenter to ensure Pop Shop is not running.
2. Ensure that caf11a8 is installed via staging apt branch
3. Run io.elementary.appcenter to launch a new instance of Pop Shop
4. App will crash when loading the window.

[isantop]

@ids1024 Running Pop_shop with either Valgrind or AddressSanitizer seems to prevent whatever race condition is causing the segfault. It does still crash if run with gdb (and compiled with debug symbols)

[isantop]

I believe I've worked around the segfault I was getting as of b0040c7 but would appreciate another check

[isantop]

After updating and rebooting, I am seeing the Pop!_Shop take about 10 seconds to open and another 5 seconds before the "Pop!_Shop" is not responding dialogue goes away. There are a few seconds between when I launch and when the dock shows the app as "running," even before the window appears. The times are consistent; if I close the window and re-open it immediately after, it still takes that long to start.

Before this update, the same system shows the window open immediately after launching, so this seems like a regression.

It might be possible to reduce the delay before the window opens, but a lot of the delay is related to setting up the async processing (which has to happen with locks in place to prevent segfaults like those already discussed in this thread) which wouldn't be possible to fix without major changes (i.e. rewrites) from upstream. It's worth noting that a lot of this processing also had to take place with the previous version, but because there was less asynchronous processing the application window would lock instead.

One other point to consider; on my system loading the currently-release version of Shop takes approximately 4 seconds from a cold start. It opens instantly if the daemon has been started (i.e. with --silent), but that's because the loading has taken place already. Ensure that Shop has closed completely (killall io.elementary.appcenter) in both cases before comparing performance between versions, as this branch also opens quickly after rebooting (since it will start in the background instead of the old version).

[mmstick]

With these changes I've not seen any crashes.

[mmstick]

I've done a lot of installing, reinstalling, and applying updates since yesterday and this morning. I've not been able to reproduce any crashes from before. Sometimes a category is a bit slow to load, and one of the locks can cause the UI to block for 5-15 seconds until a thread has finished its task, but other than that it's a significant improvement and resolves thread-safety issues in the current release.

Further improvements will likely require substantial changes to the code. The easiest improvement might be using an async variant of a mutex, using either the AsyncMutex class in the repository or making a custom one with a thread and callback. But it'd require altering every function containing an async mutex to be async too.

The biggest improvement would be to ensure that all GObjects touching anything GTK-related are only scheduled and interacted with from the global default glib main context, but the data seems very tightly coupled with their GTK widgets so that'd require a radically different architecture separating the data from the widgets.

[jacobgkau]

I tested aba46f3 (completely rebooted after installing the update to this version.) The app window opens up almost immediately after the latest updates, so that takes care of the concern regarding lack of launch responsiveness.

I'm not a fan of how the Uninstall/Open buttons on an installed app are now on top of each other instead of side-by-side, but don't have matching widths. Before:

After:

The button to get to the updates/installed apps section no longer has text inside of it, so it might be harder to find, although it has a tooltip showing what it is. Before:

After:

I would like to get a sign-off that these changes are okay from @pop-os/ux before approving this.

Also, while doing regression testing, I'm having trouble installing .deb apps. I tried installing Kdenlive as well as GNOME 2048, both of which default to Flatpak. After changing the drop-down to .deb and clicking Install, the button changes to Uninstall after about a second, but no Open button appears; there is also no loading feedback after the button changes. apt policy shows that the apps are not installed, despite the Pop!_Shop showing an Uninstall button and appearing to be finished installing.

The Pop!_Shop then gives an error (and switches the button back to Install) after attempting to uninstall.

If I close and open the Pop!_Shop after "installing," the button does not go back to Install, it stays as Uninstall. None of these issues happen with Flatpak apps (those seem to install fine, although it looks like they may not be removed properly when uninstalled.) I confirmed that I am able to install .debs with the currently released Pop!_Shop, so this is a regression. I noticed that I'm not prompted for authentication when I try to install a .deb on aba46f3, so that might be part of the issue.

[jacobgkau]

I would like QA to double check me, but If I open pop shop with this, then use the search bar on any page other than home the app completely freezes and I have to force quit it.

I saw momentary freezes when searching on the category pages, but it recovered after 5-10 seconds for me. That issue seemed less pressing than the failures to install .deb and uninstall Flatpaks, and isn't necessarily blocking compared to the current version (unless it's actually worse than what I saw), but would be nice to fix.

I know that previous versions of pop shop just disabled search on pages that weren't the home page.

Currently, we have searching within category pages as an item on the regression testing checklist, so it's supposed to work (filtering the search to only the current category). I do see the search bar correctly get disabled on the Installed page.

[maria-komarova]

Looking at the changes, the feedback from @jacobgkau seems very relevant.

• If possible, it would be much better to have the Uninstall and Open buttons be next to each other instead of stacked since it makes the UI look unpolished and gets visually in the way of the user.
• It would also be better to have a label inside of the button for Installed apps. Maybe having the button with a label "Installed" with the number overlay for the updates, the way it works now, instead of the icon button, would be enough to improve usability. The icon in the updated design is too vague to be easily recognizable and while the tooltip helps a little, it does not seem sufficient for the easy discovery of this important app functionality.
• Install UX as described by @jacobgkau does seem to be a regression.

Let us know if we should look at anything else.

[isantop]

@maria-komarova After playing around with the top bar, having the label inside of the button for Installed Apps will cause the window to automatically expand when it's being displayed at very narrow sizes due to the way hiding and showing both the Installed Apps and Back buttons works. In order to prevent this, it would be required to set a larger minimum size for the window, which could affect tiling usability.

EDIT: I'd also point out that if there are updates available, the number badge is correctly displayed on the current image button.

[isantop]

Another alternative might be using a different, symbolic icon, which better matches the rest of the Pop UI/UX. That would also be a significantly smaller change from upstream, which makes future maintainability much easier.

[maria-komarova]

Thanks for checking what would happen @isantop. What would be the minimum window width with the label instead of the icon? Could you screenshot both versions?

[isantop]

It looks like I can fit "In" before the button grows in size and begins limiting the minimum width. The button would need to be as narrow as possible (down to just being the same width as its height).

Here's a screenshot using the symbolic icon; it could also be a different icon in our set:

One other option I just considered was moving the entire button to the menu with a text item. That would also allow for a keyboard shortcut, which could be faster. Since we now have the option for automatic updates, I suspect that using the installed view is going to be much lower priority going forward, so that could simplify the top bar quite a bit and possibly allow for an even narrower window width.

[maria-komarova]

It might be an option although it is still less discoverable. It might then be better as an item below the header area but that would be even more of a patch.

[isantop]

It might be an option although it is still less discoverable. It might then be better as an item below the header area but that would be even more of a patch.

@maria-komarova I'm not sure what you mean by below the header?

[maria-komarova]

Sorry for not being clear. I meant below the banner image with Unleash your potential. But it doesn't necessarily fit there as naturally. And more of a patch.

[isantop]

Apt packages should now be fixed as of 35ac918

[isantop]

Sorry for not being clear. I meant below the banner image with Unleash your potential. But it doesn't necessarily fit there as naturally. And more of a patch.

@maria-komarova I definitely agree it doesn't fit there.

The more I think about it, the more I think that putting it into the menu makes sense. With the new updates system provided by Pop Updates, the Installed Apps page has very little actual use in Pop Shop, so I think putting in the slightly less discoverable place in the name of preserving our existing usability makes sense.

[maria-komarova]

To clarify, what is the functionality that's now left in the Installed section?

[isantop]

To clarify, what is the functionality that;s now left in the Installed section?

I think the primary functionality is for quickly getting to apps that you have installed, e.g. to uninstall them. I do think it's helpful to be able to quickly list all installed applications, and I think that appears to pan out by the universal presence of such a list in other platforms' store interfaces as well. It does appear that it's very common for other store interfaces to place the access to such a list within a hamburger menu, so I don't think the discoverability is actually that low there. I think that since elementary currently uses Appcenter as an interface not just for installing applications but also as the primary system update location, the more prominent positioning there makes sense, but since we've offloaded that function in Pop it might be more beneficial in our case to declutter the top bar, especially since the new search bar takes up so much space.

[isantop]

I've set up what this type of thing could look like. I'm not tied to the "Installed Apps" text; perhaps "Installed Software" works better?

[jacobgkau]

The more I think about it, the more I think that putting it into the menu makes sense. With the new updates system provided by Pop Updates, the Installed Apps page has very little actual use in Pop Shop,

I want to point out that auto-updates are disabled by default and not all users will enable them. The updating functionality in the Pop!_Shop is still just as relevant for those users as it was previously, regardless of the auto-updater.

[isantop]

Ah, I was under the impression that the updater was still handling the updates even with auto updates disabled.

I can put the updater badge on the menu button, which should still aid discoverability for that important function.

[isantop]

And here we are with the Updates Badge on the menu button:

[maria-komarova]

Thanks. That might be an option but we still worry about it being easy to understand how to find installed apps. Would it be possible to see how narrow we can get the window with the button with a label? Just want to see what it looks like before making the final call.

[isantop]

With an installed apps button and a label set to "Installed & Updates", the home page can shrink to a minimum width of 467 pixels, and grows to a width of 567 when opening any item from the home page.

With the label set to only "Installed", the window can shrink to a minimum of 400 px, but grows to 500 pixels upon opening any item:

I also don't feel that just the word "Installed" really conveys what the button does either, requiring the user to experiment by pressing the button anyway to discover the functionality.

For tiling compatibility, Pop Shop currently has a hard-maximum for it's minimum width of 480 pixels (one quarter of the width of a 1080p display, 1920/4), so the extra width does not work with the current design goals. Without a button, the widest the window needs is 424 pixels, which does fit within the limits.

[isantop]

One other thing to note is that the label tends to be shorter in English. If a user has their locale set to a different language where that string is translated, it will often further expand the window width. For example, in Swedish the string might be "Installerad", Spanish could be "Instalado", and German could be "Eingerichtet", all of which would increase the minimum width significantly.

[jacobgkau]

I'm not understanding why the minimum width has to grow when an item is selected vs. the home page, only when the button has text in it. The button having text and what's in the rest of the window don't seem related. Is that just a separate bug?

[isantop]

I'm not understanding why the minimum width has to grow when an item is selected vs. the home page, only when the button has text in it. The button having text and what's in the rest of the window don't seem related. Is that just a separate bug?

It grows because the button doesn't hide at the same time as the back button being shown, which means that space needs to be allocated for the back button, the search bar, the Updates button, and the menu button all simultaneously, like so:

(They wouldn't normally be actually visible at the same time, but because of the way GTK packs widgets there can be space allocated for them without them being visible yet/anymore)

[13r0ck]

Just to throw i my two cents, I prefer https://github.com/pop-os/shop/pull/401#issuecomment-1422933022 I see no benefit for showing a separate updates button that disappears when less than a specified width. It seems like it could be more confusing to have the button "randomly" disappear. I would rather have the one place to go to for updates, and have it always there.

[maria-komarova]

... a separate updates button that disappears when less than a specified width.

I didn't realize a button would disappear. We were always talking about Installed button as being permanently in the header.

@isantop thanks a lot for clarifying the situation with the window width, it helps to better understand the context. From everything that has been said it looks like Installed Apps in the Hamburger menu with the badge might be our best bet right now. Wonder if it might be helpful to have it as the first item separated from others with the divider and with the same number badge to indicate what item in the menu the badge belongs to. We can probably say "Installed Apps & Updates" with the number badge going in the top right corner of the word Updates. And maybe keep Check for Updates instead of Refresh.

[isantop]

To be clear, the button would not disappear. That was a misunderstanding on Brock's part, which I cleared up irl. The button would be a permanent part of the headerbar.

For the updates badge, I think having two of them in such close proximity would be too cluttered/redundant. What about setting an alternate background color on that item?

I can move the Updates item first in the list and add a separator.

[maria-komarova]

It might look cluttered but I don't think it would be redundant. Changing the background might not be enough to understand how one relates to another. An alternative might be to not have a keyboard shortcut and show the badge at the end of the line instead of the keyboard shortcut.

[isantop]

@maria-komarova Actually, it looks like GTK doesn't want to let me add the updates badge to the menu as well. It doesn't seem to like overlay widgets the same way as the headerbar, and is not showing either way.

I did do a quick and dirty look of what it looks like with the item background changed. A different color could also work; this just matches the color to the badge color, to help tie the two together. I have not yet moved it to the top, nor added the separator:

[isantop]

Updated to get the order/spacing correct:

[isantop]

Maybe limiting the background to the label:

[maria-komarova]

Can work if the background is the same color as the badge. I like it more when it includes the shortcut.

[isantop]

@maria-komarova So the current iteration is as follows:

@13r0ck brought up a good point that we currently use red highlights for destructive actions, whereas green is used for suggested actions. In light of this, we thought about changing both the badge and the background to green?

I do believe this improves the consistency of our color-messaging. It also reinforces that updates are good, and should be installed, whereas the red color does carry a somewhat negative connotation (like caution, warning, error, etc.)

[maria-komarova]

Good point. Green is much better.