Skip to content

Conversation

pixelspark
Copy link
Contributor

Purpose

Wrap access to Model for users that use the syncthing Go package. See discussion: #9619 (review)

Testing

It works with the iOS app. Other than that, there are no current users of this API (to my knowledge) as Model was only exposed recently form the iOS app.

Screenshots

n/a

Documentation

n/a (not user visible)

Authorship

Already in AUTHORS

@pixelspark
Copy link
Contributor Author

@imsodin I have moved the wrapper over to lib/syncthing as you suggested.

I have yet to look into narrowing the scope of this wrapper (i.e. as discussed use FolderSummaryService from the iOS app would remove the need to expose DBSnapshot).

My suggestion would be to iterate, i.e. review and merge this now, and I'll have a go at narrowing later.

@imsodin
Copy link
Member

imsodin commented Aug 10, 2024

Iteration seems fine. Which also means whatever we merge might get released, thus could we please already use the "internal" naming that was brought up before? Basically what you call model or modelwrapper now, lets call that internal.

And add a comment on it in App, something like this (very welcome to (heavily) change this, just what came to mind quickly): Internal provides access to internal workings of syncthing. Unlike other exported methods on App, this is not intended to be generally safe to use and be stable (behaviour and API wise).
Just saw you have something similar on the type - should read the full thing before commenting :)
Could you also add some comment on the App member and/or move the type comment please.

Copy link
Member

@imsodin imsodin left a comment

Choose a reason for hiding this comment

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

Some naming comments in the review. Few of them I feel a bit stronger about, few of them are just weekly held opinions - the latter begin with "optional:", if you disagree feel free to just e.g. downvote (to ack you saw it) and otherwise ignore, no need to respond why or anything.

Well turned out to be just two comments :)
Otherwise looks good. Would very much like to see moving some of your utility functions into lib/syncthing and reducing the internals surface. Though also the more I look at it, the less bad it looks as is already :)

@pixelspark
Copy link
Contributor Author

@imsodin I made some changes, how do you like it now?

@imsodin imsodin changed the title lib/syncthing: add wrapper for access to model lib/syncthing: Add wrapper for access to model Aug 11, 2024
@imsodin imsodin merged commit 9cde068 into syncthing:main Aug 11, 2024
20 of 21 checks passed
@pixelspark pixelspark deleted the model-wrapper branch August 11, 2024 18:50
@calmh calmh added this to the v1.27.11 milestone Aug 13, 2024
calmh added a commit to calmh/syncthing that referenced this pull request Aug 19, 2024
* main: (46 commits)
  build: use Go 1.23, require minimum 1.22 (syncthing#9651)
  gui, man, authors: Update docs, translations, and contributors
  lib/fs: Put the caseFS as the outermost layer (syncthing#9648)
  gui: Add Irish (ga) translation template (syncthing#9646)
  gui, man, authors: Update docs, translations, and contributors
  lib/syncthing: Add wrapper for access to model (syncthing#9627)
  cli: Remove `go-shlex` dependency (syncthing#9644)
  lib/sha256: Remove it (syncthing#9643)
  build: Update dependencies (syncthing#9640)
  Chmod -x non-executable files (fixes syncthing#9629) (syncthing#9630)
  gui, man, authors: Update docs, translations, and contributors
  all: minimal set of changes for iOS app (syncthing#9619)
  gui, man, authors: Update docs, translations, and contributors
  gui, man, authors: Update docs, translations, and contributors
  etc: Remove restart on suspend systemd service (ref syncthing#8448) (syncthing#9611)
  gui, man, authors: Update docs, translations, and contributors
  lib/fs: Add missing locks to fakeFile methods (fixes syncthing#9499) (syncthing#9603)
  lib/api: Increase test request timeout (fixes syncthing#9455) (syncthing#9602)
  gui, man, authors: Update docs, translations, and contributors
  lib/ignore: Remove unused patterns in cache (syncthing#9601)
  ...
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Aug 11, 2025
@syncthing syncthing locked and limited conversation to collaborators Aug 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants