Closed ghost closed 4 years ago
First generation from preferences.json
"Save" doesn't work yet, but that's coming
Having a single global default container carries problems of it's own. See https://github.com/kintesh/containerise/issues/53 to pick the first example I found.
For example, right now I'm logged into github with two different accounts, in two different containers. If I open an unmatched URL from this container, I'd like it to be opened in the default 'No container', but if I opened that same unmatched URL in the other container, I'd like it to remain in that same container that I'm opening it from. Same URLs in both cases, but different behaviour based on the container.
But, I notice you mention in the UI that the default container name can be a variable. Is this to allow for a future featureset where the variable is stored in the ruleset, this implementing the entry and exit rulesets discussed here?
Hi @NomDeMorte
Thank you for your response. Your focus is on entry and exit rules. I don't think default containers, as I am going to implement them, will clash with that feature request.
Since you say you're a retired dev, I think a skeleton of some python code will be simpler here (since it's closer to pseudo code). Ignore the camelcase please.
def handleUrlChange(currentContainer, oldUrl, newUrl):
if oldUrl.host == newUrl.host:
return
# Handle exit rules
newContainer = currentContainer.exitRules.getContainer(newUrl)
if newContainer:
return createTab(newUrl, newContainer)
# Handle entry rules
newContainer = getContainer(newUrl)
if newContainer:
return createTab(newUrl, newContainer)
# Default container being implemented
newContainer = getDefaultContainer(newUrl)
if newContainer:
return createTab(newUrl, newContainer)
# Default container not activated.
# Nothing to do
I can ping you once I have implemented getDefaultContainer
then you'll understand what the naming will do from the code. All it does is generate a name for the container. If you look at the examples in my first comment, you should get an idea.
Your focus is on entry and exit rules.
So is yours :) A global default container is a global exit rule.
If I'm understanding your pseudocode correctly, getDefaultContainer(newUrl)
could just call .exitRules.getContainer(newUrl)
, where .getContainer()
(the container class's member function) handles unmatched URLs - as should getContainer()
(the global function) .... That being the case, there is no need for getDefaultContainer()
to exist really.
It will be good if your implementation doesn't directly block other related feature requests... but it would also be a shame for your work to have to be removed and replaced with something else, in order to meet those requests, when the same amount of your work could instead apply some feature requests now (ie global exit rules), and have half of the work done for other feature requests (ie per-container exit rules).
I just want to make sure you are getting the best return on your investment of time, and also allowing future work to benefit from that investment.
@NomDeMorte I disagree on where getDefaultContainer
should reside. I believe it should stay global. Container exit rules can come in another PR, but they shouldn't be handling global issues only their own. Default containers are a global fallback, not a per container fallback.
This PR is already big enough as it is. Adding exit rules would make it enormous and I'd rather separate concerns.
Only have problems with the ms
variable which would make true, random, default containers possible.
I disagree on where
getDefaultContainer
should reside.
As I explained above, it does not need to exist at all, it is redundant, because getContainer() can, and should, handle it. That redundancy is the reason for this duplicated code:
# Handle entry rules
newContainer = getContainer(newUrl)
if newContainer:
return createTab(newUrl, newContainer)
# Default container being implemented
newContainer = getDefaultContainer(newUrl)
if newContainer:
return createTab(newUrl, newContainer)
Default containers are a global fallback, not a per container fallback.
That's precisely the problem. A global implementation cannot handle containers (because it is not container-aware). A per-container implementation can do both (by disregarding its own container-awareness)
If you only consider a global implementation, then it can cause you to make code which makes it harder to implement a per-container handler. If you consider both global and per-container handlers, then you can create code which allows you to do part of the work (global handler) now, and for someone else to efficiently do the rest of the job (per-container handlers) later.
I'd rather separate concerns.
That's unfortunate because they are not separate concerns. They are the same - "What should it do when we click an unmatched URL". For some people, the answer is "It should do 'something' regardless of which container I'm coming from", but for some users, the answer is "It should do 'something' depending on which container I'm coming from."
This PR is already big enough as it is. Adding exit rules would make it enormous
I'm not suggesting you fully implement per-container exit rules in this one PR. What I am pointing out, is that the most efficient way to implement both, is to consider both, and recognise that a global handler which this PR will implement, is only a small part of a bigger problem which is container-variant.
The path you are on now: Create global handler, which does not perform differently per-container <-- You stop here Remove global handler <-- Someone does this later. RIP your work, now wasted. Create per container handler <-- So they can do this. More work for them.
The path you could be on: Create per container handler, which does not perform differently per-container <-- You stop here Modify existing handler to act differently per-container <-- Someone does this later. Less work for them.
It's obvious which one is more enormous in the long-term. Edit: Formatting
@NomDeMorte feel free to fork my repo and make the changes you think are necessary.
@NomDeMorte feel free to fork my repo and make the changes you think are necessary.
That does not solve the problem I'm observing, it is the problem I'm observing. No need to write code which needs to be changed, just write it once in a way that does not require the changes.
Alright, I'm submitting it like this. The rest can be done in other PRs.
@kintesh when you find the time, I look forward to a code review.
Thanks for this @LoveIsGrief, awesome work. Containers are finally useful for their purpose of containing.
However, some problems remain. I tested your PR and an issue is that external links (going from special container->default container) behaves irradically. External links that open a new tab exit the container while external links that open in the same tab doesn't exit the container.
I'm fine with that behavior, as it keeps the backward history intact, but for other people a preference would likely be preferred. Some people will likely prefer their containers to be clean, regardless of weather a link contains a target="_blank" or not.
Another issue is that the opened external links aren't counted as child tabs in Tree Style Tabs. It seems pretty simple to fix, you just need to specify the openerTabId
option: https://github.com/piroor/treestyletab/wiki/API-for-other-addons#open-new-child-tab-from-a-specific-tab
Thanks for testing this @kristofferR
I tested your PR and an issue is that external links (going from special container->default container) behaves irradically.
Erratically? How so?
External links that open a new tab exit the container while external links that open in the same tab doesn't exit the container.
Could you describe a scenario or record it? I need more information to reproduce what you're observing in order to correct it.
Some people will likely prefer their containers to be clean, regardless of weather a link contains a target="_blank" or not.
Doesn't defining the name of containers using the {ms}
variable help? See the video I made a day or so ago. If that isn't what you're looking for, please describe a step by step scenario and what you expect to happen.
Another issue is that the opened external links aren't counted as child tabs in Tree Style Tabs. It seems pretty simple to fix, you just need to specify the openerTabId option: https://github.com/piroor/treestyletab/wiki/API-for-other-addons#open-new-child-tab-from-a-specific-tab
The openerTabId
is passed here. I too use TST and tested the extension and this pullrequest with it 🤔 What are you observing?
Haha, I figured it out. Due to a settings layout bug I completely overlooked the Save button, so I didn't really test your PR.
I've actually tested your PR now, and it works as designed as far as I can see. However, what I really want is to use the default "no container"-container as the default container. For several reasons - vertical space in the toolbar is at a huge premium on my laptop, and there's no point of marking/color coding almost everything as the default.
Is that possible right now or still unimplemented?
I just really suck at design and layout @kristofferR 😆 Dunno where to put the save button to make it obvious or which color to give it.
As for a using "No container" as a default, I have seen people using @.*
as a rule there, so you can already do that today.
You did avert me to using "No container" in Default Container > Container name that wouldn't work. Once fixed, you would be able to use this feature for that purpose too.
If you want to keep the save button on the side, it should at least be four times as wide. The "Save" text should also be horizontal, vertical text like now is almost never a good idea.
I still can't get the child tab functionality to work right. Tabs simply open in a new mother tab.
I've also opened up a new, quite related, issue (#93) about how the "Keep old tabs" works. It has a video of this PR, demonstrating how TST doesn't work properly and how the temp pages spawned with CTRL/Command click (and kept by the "Keep old tabs" preference) can be quite annoying.
@kristofferR could you record what's going wrong with TST? These are my results
I'll modify the button next week, when I have time. Thanks for the feedback!
As you can see, the links I click doesn't open in child tabs.
Cheers. I'll try to recreate it.
What about save on change and no button at all?
Cheers
I think I'll go for the "save on change". It might be more intuitive indeed
Save on change is bad because if a user clicks something in error it's done. That's why Save/apply/close/etc buttons are standard.
Thanks for implementing support for No container, but unfortunately it's really wonky. It seems to still use the container functionality despite it's not being needed. The pages are quickly closed and reloaded, and the back button/history doesn't work at all.
Would it be possible for it to instead completely bypass the container functionality so that pages that open without a container function as normal?
@kristofferR It should work now.
Hey, out of context but I want to try this. Can you publish compiled version of this branch? I want to use it. Thank you.
@sungerbob this is the one I'm currently using containerise-3.5.3alpha.zip. I uploaded it to IPFS. If it goes down I'll find another host. It could be interesting to add builds to the CI :thinking: (but that would be in another PR)
dev TODO
Visuable toggle could be done with checkbox and CSS that hides, greys out or deactivates everything in the group
ms
variableChoicePreference
with radio buttons)TODO future PRs
Discussion questions:
This will contain the work on default containers as well as their preferences.
It will introduce the options for default containers described in #10 and #80 (possibly other related issues if I remember/find them again).
What is this "default container" you speak of
The container into which tabs will be placed that cannot be placed into any other container. In other words, if a tab opens or updates its URL to one that cannot be matched by a rule in the extension, it will be put into a default container according to the preferences listed below.
Preferences
Since there have been multiple proposals on what should be done with the default container and they are at times mutually exclusive, it will be up to the user to decide. In order to do so a group of preferences related to the default container will be provided that hopefully cover the requests.
The preference group will have certain presets for the user to reach certain configuration states easily.
Suggested preference group
The title of the group, as seen by the user, will be Default container. The entire group can be toggled on or off. If off, nothing will be done with unmatched URLs. Once activated the following preferences will come into effect:
Container name (string input)
The name the default container will possess. It will be possible to create a dynamic name using certain variables:
Other variables can be requested (most likely in feature requests).
Examples:
Lifetime (choice list)
A possible countdown e.g "60 minutes" might be possible, but that's out of the scope of this PR.
Rule addition (string input)
This will create an a rule for the container. This is specifically for #80. Containers could thus be automatically created for domains.
Available variables:
Examples:
Suggested presets for the preference group
"Random, temporary containers"
By default, any unmatched URL would be put into a newly minted, unique, random container that destructs after the last tab in the container is removed.
"Autocreate containers per domain"
Close #80
"Default container"
All unmatched URLs would end up in the same container. If I'm not mistaken this would fix #10
"Default temporary container"
Same as "Default container" with the change that once the last tab in the default container is closed, the default container would be closed. Fear not, the default container would be created again once an unmatched URL is encountered.