search

skiff-org / skiff-windows-app

Skiff's Windows app for privacy-first, end-to-end encrypted Mail, Drive, Calendar, and Pages.
https://skiff.com
GNU Affero General Public License v3.0
108 stars 70 forks source link

Added some convenience tools and goodies #4

Closed Novack closed 1 year ago

Novack commented 1 year ago

• Added an About msg box to identify current installed version. • Added an option to check for updates (w/ option to open releases url), based on the current scheme. • Added option to run on windows startup. • Added option to close to tray (adds to the existing minimize to tray). • Added option to start minimized. • Added main window auto-save position and state between sessions. • Added settings persistence.

All this options can be accessed by users through the tray icon context menu. image image image

amilich commented 1 year ago

This is amazing! Going to try it out in a minute.

Once we get this in, send me an email (andrew @ ) and I'd love to upgrade you to the highest plan on us! And, give you early access to a lot more coming out soon.

amilich commented 1 year ago

I wonder if clicking "yes" should open skiff.com/download, where there will be a Windows button that downloads the MSI automatically, instead of the latest release page. Not sure yet.

Novack commented 1 year ago

I wonder if clicking "yes" should open skiff.com/download, where there will be a Windows button that downloads the MSI automatically, instead of the latest release page. Not sure yet.

Makes sense! Is pretty trivial to change :) Im grabbing the url from Github API response, but having a fixed address would be even simpler.

amilich commented 1 year ago

I wonder if clicking "yes" should open skiff.com/download, where there will be a Windows button that downloads the MSI automatically, instead of the latest release page. Not sure yet.

Makes sense! Is pretty trivial to change :) Im grabbing the url from Github API response, but having a fixed address would be even simpler.

Definitely. I think the way it is is probably better. It's still pretty clear.

Novack commented 1 year ago

Just found a bug when trying to use the http client again. Give me two minutes. In the meantime, let me know if you changed your mind regarding the url :)

amilich commented 1 year ago

One small nit from development - if you have a later version, the updater is confused (see screenshot). Don't know if it needs to be fixed though. image

amilich commented 1 year ago

One small nit from development - if you have a later version, the updater is confused (see screenshot). Don't know if it needs to be fixed though. image

Using the API response is probably the safest thing to do.

Novack commented 1 year ago

One small nit from development - if you have a later version, the updater is confused (see screenshot). Don't know if it needs to be fixed though.

Ah yes, Im bruteforcing it: if current version differs from the latest published in github, the local is considered outdated. Is not the safest option, but did not wanted to go all the way into comparing versions without being sure that the current versioning scheme (Skiff Windows App - 1.0.0.4) is "officially adopted". I can fix it though! Should not be harsh.

Novack commented 1 year ago

Turns out comparing versions is pretty trivial, let me fix it, one more minute! 🦗

Novack commented 1 year ago

Ok that is much cleaner and safer now using System.Version 👍

One thing remains that bothers me, and is that in the github public api response, there is no concrete field for the actual version. So Im extracting it from the "tag_name" field. image

amilich commented 1 year ago

Ok that is much cleaner and safer now using System.Version 👍

One thing remains that bothers me, and is that in the github public api response, there is no concrete field for the actual version. So Im extracting it from the "tag_name" field. image

Ah, I see. I can start adding that on new versions. Let me see if I can do that for previous ones now.

amilich commented 1 year ago

Tried updating latest tag to v1.0.0.4.

Novack commented 1 year ago

nit - fine to keep these as "Skiff Desktop" because all apps are there

Oh, right! Lame on my part, was too focused on mail 😵 :)

Novack commented 1 year ago

Tried updating latest tag to v1.0.0.4.

Oh thats awesome! Would be possible to also remove the "v"?

amilich commented 1 year ago

Tried updating latest tag to v1.0.0.4.

Oh thats awesome! Would be possible to also remove the "v"?

Yes - will do that now.

amilich commented 1 year ago

Tried updating latest tag to v1.0.0.4.

Oh thats awesome! Would be possible to also remove the "v"?

Done

Novack commented 1 year ago

Tried updating latest tag to v1.0.0.4.

Oh thats awesome! Would be possible to also remove the "v"?

Done

Perfect, change pushed. Re: Skiff mail vs desktop, you edited it already, or shall I tweak it quickly and commit?

amilich commented 1 year ago

Tried updating latest tag to v1.0.0.4.

Oh thats awesome! Would be possible to also remove the "v"?

Done

Perfect, change pushed. Re: Skiff mail vs desktop, you edited it already, or shall I tweak it quickly and commit?

I have the edits and will re-push it to main once we have the new release out! Feel free to merge anytime.

amilich commented 1 year ago

Release out!

Novack commented 1 year ago

Awesome. Thank you!

riverar commented 1 year ago

@amilich The team should probably pause here and do some important housekeeping:

Novack commented 1 year ago

I agree about the coding conventions, disagree about the rest of technocratic bureocracy (unleast legally obliged). I certainly reject certain patterns as they tend to be mere snobisms around OSS.

Multifeature-PRs like this sometimes are simply based on common sense and reality. On this case, was precisely for avoiding an absurd pile of conflicting/complementing/related PRs for equally interrelated and complementing small features. If I had to introduce 6/7 consecutive PRs of really small stuff only to comply to some predefined bureocracy, I would have simply not do it, and just make a build for my own.

So in my experience, the right balance between care and pretentiousness, comes with good coding conventions guidelines, solid reviews and letting people do some work.

riverar commented 1 year ago

It's not clear which suggestions you consider snobisms so I'll generally expand on a few of my suggestions below. My opinions here are driven from my experience shipping Windows apps for the past 20 years, having an active OSS app across 6 million users, and co-authoring a number of Windows books.

One feature per commit/PR, within reason

Skiff Desktop is a security-critical piece of software so patterns, such as one-feature-per-commit/PR, are crucial for quick and complete review. It also lets folks quickly revert changes if needed; as it currently stands, reverting this PR removes a large number of features.

That said, I agree that the overhead isn't worth the trouble in some cases. If I was to break up this PR into separate PRs, it would look something like (in no order):

Contributor agreements, a necessary legal evil?

I am not a lawyer, but I believe the Contributor License Agreement (CLA) is critical to establish who owns what. In the US, you would likely own the code in this PR and the Skiff codebase would have a mixture of ownership. Skiff is likely now legally required to ask for your permission for various tasks, such as re-licensing this project. It's easy to take care of early in the repository lifecycle. It's a monster later.

Style guide

Following the Microsoft style guide ensures we're all talking the same language and creates consistency between the app and operating system. For example, the notifications area next to the clock is referred to as the notification area (Windows 10 and below) or the system tray (Windows 11 and up). So the context menu text Minimize to Tray would ideally say Minimize to system tray or Minimize to notification area, depending on the operating systems Skiff Desktop targets. (In this case, I would use the former option, as the system tray is commonly understood slang prior to Windows 11.)

(I wouldn't change what you have right now, but we should strongly consider the impact of adding any new UI or text to the app in the future. Every string and bit of UI added to the app creates localization and documentation debt.)

Novack commented 1 year ago

Ugh. This is is exactly what I mean. Sorry mate, not reading your essay, had my fair share of internet debates like 30 years ago. Nowadays I rather focus on doing actual work with my given time :)

Cheers!

riverar commented 1 year ago

@Novack I would consider revisiting it when you have time, you might learn something new.

Novack commented 1 year ago

@riverar your whole behaviour has been so petulant and narcissistic, that I know for a fact that regardless of your self-perceived superiority, you have nothing to teach. Not me, not anyone. Consider steping down from your ego when you can, you might learn something new.

riverar commented 1 year ago

@Novack Sincere apologies if any of my messages came across as negative, that was not my intent. I'll reflect on this thread and figure out how I could have better presented my suggestions. Thanks for your patience! (Email is my profile if you'd like to chat offline too.)

amilich commented 1 year ago

Hey everyone :)

There is a lot of progress to be made across the board, both in standards and in feature completeness. We welcome all sorts of contributions, including small feature updates and code style updates. We also have a contributing guide in https://github.com/skiff-org/skiff-apps/tree/main that will be applied here as well.

Novack commented 1 year ago

Hey everyone :)

@amilich Apologies for the derailment here. 🙏