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

Add ability to customize automatic update channels for add-ons #17597

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions source/addonStore/dataManager.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2022-2024 NV Access Limited
# Copyright (C) 2022-2025 NV Access Limited
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.

Expand Down Expand Up @@ -44,6 +44,8 @@
_getCacheHashURL,
_LATEST_API_VER,
)
from .settings import _AddonStoreSettings


if TYPE_CHECKING:
from addonHandler import Addon as AddonHandlerModel # noqa: F401
Expand Down Expand Up @@ -98,6 +100,7 @@ def __init__(self):
pathlib.Path(WritePaths.addonStoreDir).mkdir(parents=True, exist_ok=True)
pathlib.Path(self._installedAddonDataCacheDir).mkdir(parents=True, exist_ok=True)

self.storeSettings = _AddonStoreSettings()
self._latestAddonCache = self._getCachedAddonData(self._cacheLatestFile)
self._compatibleAddonCache = self._getCachedAddonData(self._cacheCompatibleFile)
self._installedAddonsCache = _InstalledAddonsCache()
Expand All @@ -110,6 +113,7 @@ def __init__(self):
self._initialiseAvailableAddonsThread.start()

def terminate(self):
self.storeSettings.save()
if self._initialiseAvailableAddonsThread.is_alive():
self._initialiseAvailableAddonsThread.join(timeout=1)
if self._initialiseAvailableAddonsThread.is_alive():
Expand Down Expand Up @@ -349,17 +353,27 @@ def _getCachedInstalledAddonData(self, addonId: str) -> Optional[InstalledAddonS
return _createInstalledStoreModelFromData(cacheData)

def _addonsPendingUpdate(self) -> list["_AddonGUIModel"]:
# TODO: Add AvailableAddonStatus.UPDATE_INCOMPATIBLE,
# to allow updates that are incompatible with the current NVDA version,
# only if a config setting is enabled
updatableAddonStatuses = {AvailableAddonStatus.UPDATE}
addonsPendingUpdate: list["_AddonGUIModel"] = []
compatibleAddons = self.getLatestCompatibleAddons()
for channel in compatibleAddons:
for addon in compatibleAddons[channel].values():
if (
getStatus(addon, _StatusFilterKey.UPDATE) == AvailableAddonStatus.UPDATE
# Only consider add-ons that have been installed through the Add-on Store
and addon._addonHandlerModel._addonStoreData is not None
):
# Only consider add-on updates for the same channel
if addon.channel == addon._addonHandlerModel._addonStoreData.channel:
if getStatus(addon, _StatusFilterKey.UPDATE) in updatableAddonStatuses:
# Ensure add-on update channel is within the preferred update channels
if (installedStoreData := addon._addonHandlerModel._addonStoreData) is not None:
installedChannel = installedStoreData.channel
else:
installedChannel = Channel.EXTERNAL
selectedUpdateChannel = addonDataManager.storeSettings.getAddonSettings(
addon.addonId,
).updateChannel
availableUpdateChannels = selectedUpdateChannel._availableChannelsForAddonWithChannel(
installedChannel,
)
if addon.channel in availableUpdateChannels:
addonsPendingUpdate.append(addon)
return addonsPendingUpdate

Expand Down
96 changes: 94 additions & 2 deletions source/addonStore/models/channel.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2022-2023 NV Access Limited
# Copyright (C) 2022-2025 NV Access Limited
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.

Expand All @@ -9,7 +9,8 @@
Set,
)

from utils.displayString import DisplayStringStrEnum
import config
from utils.displayString import DisplayStringIntEnum, DisplayStringStrEnum


class Channel(DisplayStringStrEnum):
Expand Down Expand Up @@ -52,3 +53,94 @@ def _displayStringLabels(self) -> Dict["Channel", str]:
"""A dictionary where the keys are channel groups to filter by,
and the values are which channels should be shown for a given filter.
"""


class UpdateChannel(DisplayStringIntEnum):
"""Update channel for an addon used for automatic updates."""

DEFAULT = -1
"""Default channel.
Specified in [addonStore][defaultUpdateChannel] section of config.
"""

SAME = 0
"""Keep the same channel as the current version"""

ANY = 1
"""Use any channel, keep to the latest version"""

NO_UPDATE = 2
"""Do not update the addon"""

STABLE = 3
"""Use the stable channel"""

BETA_DEV = 4
"""Use the beta or development channel, keep to the latest version"""

BETA = 5
"""Use the beta channel"""

DEV = 6
"""Use the development channel"""

@property
def displayString(self) -> str:
# Handle the default channel separately to avoid recursive dependency
# on _displayStringLabels.
if self is UpdateChannel.DEFAULT:
channel = UpdateChannel(config.conf["addonStore"]["defaultUpdateChannel"])
assert channel is not UpdateChannel.DEFAULT
# Translators: Update channel for an addon.
# {defaultChannel} will be replaced with the name of the channel the user has selected as default
return _("Default ({defaultChannel})").format(
defaultChannel=self._displayStringLabels[channel],
)
return super().displayString

@property
def _displayStringLabels(self) -> dict["UpdateChannel", str]:
return {
# Translators: Update channel for an addon.
# Same means an add-on only updates to a newer version in the same channel.
# e.g. an installed beta version only updates to beta versions.
UpdateChannel.SAME: _("Same"),
# Translators: Update channel for an addon.
# Any means an add-on updates to the latest version regardless of the channel.
UpdateChannel.ANY: _("Any"),
# Translators: Update channel for an addon
UpdateChannel.NO_UPDATE: _("Do not update"),
# Translators: Update channel for an addon
UpdateChannel.STABLE: _("Stable"),
# Translators: Update channel for an addon
UpdateChannel.BETA_DEV: _("Beta or dev"),
# Translators: Update channel for an addon
UpdateChannel.BETA: _("Beta"),
# Translators: Update channel for an addon
UpdateChannel.DEV: _("Dev"),
}

def _availableChannelsForAddonWithChannel(self, addonChannel: Channel) -> set[Channel]:
"""Return the available update channels for an addon with the given channel and the update channel set."""
if self == UpdateChannel.DEFAULT:
channel = UpdateChannel(config.conf["addonStore"]["defaultUpdateChannel"])
assert channel is not UpdateChannel.DEFAULT
else:
channel = self
match channel:
case UpdateChannel.SAME:
return {addonChannel}
case UpdateChannel.ANY:
return _channelFilters[Channel.ALL]
case UpdateChannel.NO_UPDATE:
return {}
case UpdateChannel.STABLE:
return {Channel.STABLE}
case UpdateChannel.BETA_DEV:
return {Channel.BETA, Channel.DEV}
case UpdateChannel.BETA:
return {Channel.BETA}
case UpdateChannel.DEV:
return {Channel.DEV}
case _:
raise ValueError(f"Invalid update channel: {self}")
1 change: 1 addition & 0 deletions source/addonStore/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ def _downloadAddonToPath(
False if the download is cancelled
"""
if not NVDAState.shouldWriteToDisk():
log.error("Should not write to disk, cancelling download")
return False

# Some add-ons are quite large, so we need to allow for a long download time.
Expand Down
137 changes: 137 additions & 0 deletions source/addonStore/settings.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2025 NV Access Limited
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.

from dataclasses import dataclass, replace
import json
import os

from logHandler import log
import NVDAState

from .models.channel import UpdateChannel


@dataclass
class _AddonSettings:
"""Settings for the Add-on Store management of an add-on.

All options must have a default value.
"""

updateChannel: UpdateChannel = UpdateChannel.DEFAULT
"""Preferred update channels for the add-on."""

# TODO: migrate enabled/disabled/blocked state tracking
# from addonHandler.AddonState/AddonStateCategory to here.
# The set based state tracking could be replaced by maintaining state data on each add-on.
#
# blocked: bool = False
# """Whether the add-on is blocked from being running due to incompatibility."""
#
# disabled: bool = False
# """Whether the add-on is disabled."""
Comment on lines +26 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Has this intentionally not been done?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, out of scope for this PR

Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue for it? If not, can you create one?



class _AddonStoreSettings:
"""Settings for the Add-on Store."""

_CACHE_FILENAME: str = "_cachedSettings.json"

_showWarning: bool
"""Show warning when opening Add-on Store."""

_addonSettings: dict[str, _AddonSettings]
"""Settings related to the management of add-ons"""

def __init__(self):
self._storeSettingsFile = os.path.join(
NVDAState.WritePaths.addonStoreDir,
self._CACHE_FILENAME,
)
self._showWarning = True
self._addonSettings = {}
self.load()

def load(self):
settingsDict = {
"showWarning": True,
"addonSettings": {},
}
Comment on lines +58 to +61
Copy link
Member

Choose a reason for hiding this comment

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

The defaults are already given at lines 59-60, it seems WET to repeat them here.
Also, given this, surely we can just return from this function if the cached settings are invalid?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, bit lost here. Where else are these values defined? can you link the code?
Do you mean returning rather than removing the cached file?

Copy link
Member

Choose a reason for hiding this comment

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

In _AddonStoreSettings.__init__, you already define the default value of _showWarning and _addonSettings.

self._showWarning = True
self._addonSettings = {}

So it probably makes sense to

  1. Return if a settings cache is not found (
    except FileNotFoundError:
    pass
    )
  2. Return if the settings cache is not JSON (
    except Exception:
    log.exception("Invalid add-on store settings")
    if NVDAState.shouldWriteToDisk():
    os.remove(self._storeSettingsFile)
    )
  3. Return if the types are invalid (
    except (KeyError, ValueError):
    log.exception(f"Invalid add-on store cache:\n{settingsDict}")
    if NVDAState.shouldWriteToDisk():
    os.remove(self._storeSettingsFile)
    )

try:
with open(self._storeSettingsFile, "r", encoding="utf-8") as storeSettingsFile:
settingsDict = json.load(storeSettingsFile)
except FileNotFoundError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pass
return

except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

According to the docs, json.load raises JSONDecodeError or UnicodeDecodeError. Can we be more specific with what we catch here?

log.exception("Invalid add-on store settings")
if NVDAState.shouldWriteToDisk():
os.remove(self._storeSettingsFile)
SaschaCowley marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
os.remove(self._storeSettingsFile)
os.remove(self._storeSettingsFile)
return

try:
if not isinstance(settingsDict["addonSettings"], dict):
raise ValueError("addonSettings must be a dict")

if not isinstance(settingsDict["showWarning"], bool):
raise ValueError("showWarning must be a bool")

except (KeyError, ValueError):
log.exception(f"Invalid add-on store cache:\n{settingsDict}")
if NVDAState.shouldWriteToDisk():
os.remove(self._storeSettingsFile)
SaschaCowley marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
os.remove(self._storeSettingsFile)
os.remove(self._storeSettingsFile)
return


self._showWarning = settingsDict["showWarning"]
for addonId, settings in settingsDict["addonSettings"].items():
try:
updateChannel = UpdateChannel(settings["updateChannel"])
except ValueError:
log.exception(f"Invalid add-on settings for {addonId}:\n{settings}. Ignoring settings")
continue
else:
self._addonSettings[addonId] = _AddonSettings(
updateChannel=updateChannel,
)

def save(self):
if not NVDAState.shouldWriteToDisk():
log.error("Shouldn't write to disk, not saving add-on store settings")
return
seanbudd marked this conversation as resolved.
Show resolved Hide resolved
settingsDict = {
"showWarning": self._showWarning,
"addonSettings": {
addonId: {
"updateChannel": addonSettings.updateChannel.value,
}
for addonId, addonSettings in self._addonSettings.items()
},
}
with open(self._storeSettingsFile, "w", encoding="utf-8") as storeSettingsFile:
json.dump(settingsDict, storeSettingsFile, ensure_ascii=False)
SaschaCowley marked this conversation as resolved.
Show resolved Hide resolved

def setAddonSettings(self, addonId: str, **kwargs):
"""Set settings for an add-on.

Keyword arguments the same as _AddonSettings:
- updateChannel: Update channel for the add-on.
"""
if addonId not in self._addonSettings:
self._addonSettings[addonId] = _AddonSettings(**kwargs)
else:
self._addonSettings[addonId] = replace(self._addonSettings[addonId], **kwargs)
self.save()
SaschaCowley marked this conversation as resolved.
Show resolved Hide resolved

def getAddonSettings(self, addonId: str) -> _AddonSettings:
"""Get settings for an add-on.

Returns default settings if the add-on has no stored settings.
"""
return self._addonSettings.get(addonId, _AddonSettings())

@property
def showWarning(self) -> bool:
return self._showWarning

@showWarning.setter
def showWarning(self, showWarning: bool):
self._showWarning = showWarning
self.save()
SaschaCowley marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 5 additions & 3 deletions source/config/configSpec.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2006-2024 NV Access Limited, Babbage B.V., Davy Kager, Bill Dengler, Julien Cochuyt,
# Copyright (C) 2006-2025 NV Access Limited, Babbage B.V., Davy Kager, Bill Dengler, Julien Cochuyt,
# Joseph Lee, Dawid Pieper, mltony, Bram Duvigneau, Cyrille Bougot, Rob Meredith,
# Burman's Computer and Education Ltd., Leonard de Ruijter, Łukasz Golonka
# This file is covered by the GNU General Public License.
Expand All @@ -13,7 +13,7 @@
#: provide an upgrade step (@see profileUpgradeSteps.py). An upgrade step does not need to be added when
#: just adding a new element to (or removing from) the schema, only when old versions of the config
#: (conforming to old schema versions) will not work correctly with the new schema.
latestSchemaVersion = 14
latestSchemaVersion = 15
Copy link
Member

Choose a reason for hiding this comment

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

This may need to be increased if #17505 is merged first.


#: The configuration specification string
#: @type: String
Expand Down Expand Up @@ -335,9 +335,11 @@
playErrorSound = integer(0, 1, default=0)

[addonStore]
showWarning = boolean(default=true)
automaticUpdates = option("notify", "disabled", default="notify")
baseServerURL = string(default="")
# UpdateChannel values:
# same channel (default), any channel, do not update, stable, beta & dev, beta, dev
defaultUpdateChannel = integer(0, 6, default=0)
"""

#: The configuration specification
Expand Down
14 changes: 13 additions & 1 deletion source/config/profileUpgradeSteps.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# A part of NonVisual Desktop Access (NVDA)
# Copyright (C) 2016-2024 NV Access Limited, Bill Dengler, Cyrille Bougot, Łukasz Golonka, Leonard de Ruijter
# Copyright (C) 2016-2025 NV Access Limited, Bill Dengler, Cyrille Bougot, Łukasz Golonka, Leonard de Ruijter
# This file is covered by the GNU General Public License.
# See the file COPYING for more details.

Expand Down Expand Up @@ -471,3 +471,15 @@ def _friendlyNameToEndpointId(friendlyName: str) -> str | None:
# Proceed to the next device state.
continue
return None


def upgradeConfigFrom_14_to_15(profile: ConfigObj) -> None:
"""
Remove the addonStore.showWarning setting.
"""
if "addonStore" in profile and "showWarning" in profile["addonStore"]:
from addonStore.dataManager import addonDataManager

addonDataManager.storeSettings.showWarning = profile["addonStore"].as_bool("showWarning")
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the config contains an invalid value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe when there is an invalid config it fails to load in NVDA and resets the users config.

Copy link
Member

Choose a reason for hiding this comment

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

Validation happens after upgrade (the old config could be invalid according to the new schema, so it logically has to happen in this order), so there's no guarantee that you'll get a bool.

del profile["addonStore"]["showWarning"]
log.debug("Removed addonStore.showWarning setting.")
Loading