[WIP]: Track recently selected languages #305
Conversation
|
Cool, thanks for this. Will have to give it a closer read, but looks nice and tidy at first glance, and thanks for the tests! (if anyone else @xi-editor/xi-mac wants to take a look here that would be welcome!) |
|
I've given this a read through, and I do see @jansol's point: it feels like there's maybe one more layer of abstraction than is necessary here? I like the idea of using a protocol for the main interface, but I did find the overall inheritance chain tricky to follow, in a way that didn't seem totally necessary. For instance: if |
|
@cmyr Yes, it makes sense. The initial idea was just to separate store interaction logic and business logic, so they both could be modified independently. But we can definitely combine this two classes and it wont make the codebase any harder to maintain. Lets keep it simple at least at this stage of project. |
|
@cmyr @jansol Guys, I have removed |
|
So languages are supposed to show as in use in both "recently used" and the general list at the same time? That seems like reasonable behavior, but I don't see where the recently used one is marked as in use in the code. Missed when committing? |
|
@jansol Hmm, no in use language is supposed to be selected only in general list section. I am sorry, but I think screenshot is misleading and I even do not know how I made it with that item selected. I will update the screenshot. |
|
Yeah, I think it's more consistent that way. |
|
@jansol Great! Thanks a lot! As for that indentation, I just had a look at macOS system implementations and the indentation is used in very rare variety of cases, so I will probably remove it. Here are few examples from system implementation: Also, do you think we need to add available languages title? I think it will be more consistent that way. What do you think? |
|
Yeah looks like not indenting is more in line with the rest of the system. (And we can always put the indentation back if it turns out problematic) I don't think a subtitle for the available languages is necessary - the menu entry that opens this menu is already called |
|
I agree that the indentation might not be necessary. I've played around with this, and I do have a couple notes:
Overall this looks good, and feels like a nice improvement; I look forward to getting it merged. 👍 |
| @@ -0,0 +1,23 @@ | |||
| // Copyright 2016 The xi-editor Authors. | |||
There was a problem hiding this comment.
@cmyr 😆 You are right. I have just copy pasted the header. We might need to have custom header and automatically add it to all newly created source files. What do think?
There was a problem hiding this comment.
This is an Xcode feature? I'd forgotten about that, it could be helpful though!
|
@cmyr I just played with the application and feature for a while and did not have a change to reproduce the issue with initial As for descending ordering I think it would be better to have it. So, I will apply change and let you know when it is ready! Thanks. |
|
@cmyr I have updated implementation. Please, have a look at it! Now most recent language appears at the top of the list and pushes previously used languages to the bottom. If one of the recently used languages is selected it is moved to the top of the list again. Thus, recently used languages list represents ordered history of languages which were used. |
There was a problem hiding this comment.
I can't reproduce my earlier issue, but I do now have an issue where I can somehow make the menu only show the recent languages?
If I open a blank document (with nsuserdefaults cleared, not sure if that is important) and select a few different languages from the list, and then I open a file with a known extension that is not one of the languages I've already chosen, the available languages disappear?
There's some other strange sync stuff in here as well; for instance on first launch, I can end up in a state where the frontmost view is assigned the incorrect language in the menu.
This stuff all feels a bit race-y; I suspect that funny things are happening when languageChanged gets called a bunch of times by core, while the frontend is still doing some setup?
In any case maybe play around and see if you can see any of these issues?
|
@cmyr Yes, I can see the issue. It appeared because I have changed Language menu building logic a bit. |
|
@demalex Ah, right, this makes sense. What we should do is handle basically: |
|
@cmyr Initially I had same thought, but then I decided to clarify this behaviour. Thanks for the explanation! Basically I renamed I do not think that it is a good idea to keep all shared objects in Nevertheless, I have updated the implementation. Please have a look and let me know what you think. Thanks! |
|
|
||
| let newLanguage = language.trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
||
| if newLanguage.count == 0 { |
There was a problem hiding this comment.
This check seems like a perfect job for a guard 😄
guard newLanguage.count > 0 else { return }
There was a problem hiding this comment.
@mmatoszko you think if can't handle it? 😎 It is not a big difference in this situation what to use. I think it easy to read simple if than guard. guard is good when you need to declare variables but do not want to increase cyclomatic complexity. Yes, compiler checks and if there is no return at some point it breaks compilation, but in such case (having tests) it is not so critical.
|
|
||
| class DictionaryStorage: KeyValueStorage { | ||
|
|
||
| var dictionary: [String : Any] |
There was a problem hiding this comment.
@mmatoszko DictionaryStorage should be used for tests only when you need to mock a class that conforms to KeyValueStorage protocol. The property is not private because sometimes during testing it is needed to check what exact values were saved to storage. Having this property public allows to do that.
|
@demalex I definitely agree that our use of app delegate is pretty sketchy, and I'm open to refactoring. One related project is #275, which would move most of the RPC handling stuff out of the app delegate as well. Have been a bit distracted the past few days, but will try to give this another proper review either tonight or tomorrow. Thanks for your patience! |
|
@cmyr Thanks a lot and take your time! Let me know if you think that something needs to be improved. I will have a look at another task for now. |
XiEditor/EditViewController.swift
Outdated
| private var currentLanguage: String = "" | ||
|
|
||
| /// Object provides available and recently used languages | ||
| var languagesProvider: LanguagesProvider! |
There was a problem hiding this comment.
Okay, so I've been thinking about this a bit:
I think it's important that we have a clear sense of what the source of truth is for language information. I'm thinking of the situation where (theoretically) a new plugin is loaded in core that adds a new language: What happens in this case? As currently implemented, the LanguagesProvider in the AppDelegate would get updated, but the individual views wouldn't end up being notified?
There are a variety of approaches to this. One simple one would be to have all access to the languagesProvider be explicitly shared; the view controller would be returning a reference to the document, which would be returning the shared object in the AppDelegate. Then when the state changed, we would iterate through the view controllers and notify them of the state change, and they can reload menus etc as needed.
Alternatively, when the state gets set in core we could go and set the new languageProvider on each view controller, and then handle any updates in a didSet method.
Another confusion here is that because LanguagesStore is a class, everyone should currently be sharing a single instance, and so the state should be in sync; but LanguagesProvider isn't declared as a class protocol, so if it were implemented as a struct this would no longer be true?
Another thing I might think about is whether this state belongs more in the document or in the view controller. I think it might be reasonable to have it only be in the document? I could go either way, though.
There was a problem hiding this comment.
@cmy Thank you for review!
You are right, class protocol constraint definitely needs to be added to LanguagesProvider 👍🏻
Ideally ViewController should not contain any state. ViewController is an entity of UI / Presentation layer and its main responsibility to be a glue between model (view-model) and view and handle view's lifecycle. I definitely would not keep state in view controller and most likely it is Document object's responsibility.
I have been thinking about it a bit and I came up with possible solution for the problem. The idea is to have LanguagesStore as single source of truth and listeners which can listen to notifications about the updates. The LanguageStore itself can be updated by receiving notification from the core. I have created high level UML diagram to make it a little bit clear. Please, have a look:
Diagram is high level in some cases (Document and XiDocumentController, Core), but still brings visibility how data flows from presentation layer to core and to LanguagesStore.
What do you think?
There was a problem hiding this comment.
So I think we're broadly on the same page; what we want is a single source of truth and a notification mechanism.
We have options in terms of what our notification mechanism is. The simplest is that when the state changes, we iterate through our documents and call some stateChanged() function; then each document goes and checks the new state and performs some update logic. Another reasonable option is to lean on the Cocoa NotificationCenter infrastructure, which you're doing here. In this case, though, I think it makes more sense to pass some reference type containing the state along as the notification's object; so in this case I would register for, say, the 'availableLanguagesChanged' notification, and the notification would contain a reference to some type, say, CoreState, (which can be a protocol) which has a member var availableLanguages { get }.
I think this is mostly what you're suggesting? The main thing I want to push is that we'd like to balance the benefits (clear separation of responsibilities, clear flow of data) against the costs of increasing architectural complexity; so whatever we do here should be something that we anticipate being the general model for communicating changes to shared state.
There was a problem hiding this comment.
@cmyr Yes, its true! This is what I am suggesting. Please, take a note that LanguageStoringNotification is actually an interface. Interface name is misleading and will be changed. Having interface makes a loose coupling and allows to do checking during compile time, while NSNotification.Name is actually a String and it is easy to break an implementation. So, I would not use NotificationCenter.
So, the basic idea is very simple:
- Subscriber implements protocol and can be subscribed for receiving updates from
LanguagesStore LanguagesStorereceives updates from theCore, updates it's state and notifies all subscribers by iterating over the collection and calling appropriate function- As you mentioned above, updated state of the store (as a type of lets say
LanguageStoringOutput) can passed to the function as parameter
What do you think? Does it make sense?
Architecture is what allows you to manage complexity and makes it easy to adapt the code base for new requirements. So, there is always some level of architecture complexity. But, I agree, there should be a trade off.
Please, let know what do you think about my suggestion. Thank you!
There was a problem hiding this comment.
Okay, thanks for the explanation. I think we're on the same page, and this approach sounds reasonable. 👍
There was a problem hiding this comment.
My one thought is that we might want to eventually expand this so that we have a single mechanism for various types of state (themes are an obvious example), so I'd keep that in mind; for instance we might want multiple types of events that can be subscribed to, or we might want them to be a package deal.
There was a problem hiding this comment.
@cmyr Thank you very much for the feedback! Yes, I am also thinking about generic solution that can be used in variety of cases. I was busy at work these days and going to have look at it on the weekend. Thanks!
| protocol LanguagesProvider { | ||
| func fetchRecentlyUsedLanguages() -> [String] | ||
| func saveRecentlyUsed(language: String) | ||
| var availableLanguages: [String] { get set } |
There was a problem hiding this comment.
I'm not sure if we really want the set method exposed here? availableLanguages should only really be set in response to a core RPC, so exposing the setter to other users feels a bit iffy.
There was a problem hiding this comment.
@cmyr You are absolutely right. I think we need to change data flow a bit. I have some ideas and will share it soon.
|
@demalex just checking in, if this is ready for a review just 🔔 me! |
|
@cmyr Sorry for the delay in answering. I am busy with work lately, but I am moving on with implementation. I hope, I can finish it soon. Let me know if it acceptable. |
|
@demalex absolutely, work at whatever pace is good for you. I'm generally more worried that things are blocked on review or some question I've missed, so wanted to make sure that wasn't the case. |
demalex
changed the title
demalex
changed the title
|
@demalex are you still making progress on this? It feels like it's really close, and it would be nice to get this feature done, but no worries if you're busy. 😃 |
|
@cmyr Hi! Excuse me that it is taking so long to complete this. I a little bit overloaded at work at the end of the year, but I am making a progress and submit a new patch set soon. Hope it works for you. |
|
@demalex amazing, glad to hear you're still hacking away. Happy holidays! |
|
Closed it due to big difference between master and feature branch. Will resubmit it soon |
|
@demalex ping? |
Description
Implementation of enhancement #299 suggested by @cmyr. Every time user makes a language selection, the language is stored to
UserDefaults.Using
UserDefaultsdirectly makes classes tightly coupled with it and makes it hard to maintain and test. To get rid of problems like this,UserDefaultswas covered with additional abstraction. The idea is to keep settings provider in one place (AppDelegatefor now) and inject it as dependency into other consumers. There are other places whereUserDefaultsis used directly and from my perspective it requires to be refactored, but such refactoring is out of this enhancement scope.SettingsProvideris responsible for communication withKeyValueStorage. It executes save and fetch operations, whileSettingsProvidingProcessordecoratesSettingsProviderand brings additional logic for save languages operation. Appropriate test cases also have been added.Screenshot