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

make navigator update "focus-tiddler" field ... #3590

Closed
wants to merge 11 commits into from

Conversation

BurningTreeC
Copy link
Contributor

... when cancelling, deleting, closing

a findAdjacentTiddler function handles which tiddler-title gets written if the closed/cancelled/deleted tiddler is the one in the "current-tiddler" field

  • the next tiddler in the river if it's present
  • else the previous one
  • if there's no next/previous tiddler nothing is changed (the last one remains in "current-tiddler")

with this, every action that changes the story list sets the current-tiddler field if the surrounding navigator widget has a history tiddler defined

... when cancelling, deleting, closing

a `findAdjacentTiddler` function handles which tiddler-title gets written if the closed/cancelled/deleted tiddler is the one in the "current-tiddler" field

- the next tiddler in the river if it's present
- else the previous one
- if there's no next/previous tiddler nothing is changed (the last one remains in "current-tiddler")

with this, every action that changes the story list sets the `current-tiddler` field if the surrounding navigator widget has a history tiddler defined
@Jermolene
Copy link
Member

Hi @BurningTreeC looks good at a brief glance. I'd like to change current-tiddler to focus-tiddler, though, to avoid confusion with the existing currentTiddler variable.

@BurningTreeC
Copy link
Contributor Author

I'd like to change current-tiddler to focus-tiddler, though, to avoid confusion with the existing currentTiddler variable

Hi @Jermolene, that's ok, I'll do that.

I've added a "focus-tiddler" stylesheet which uses bits from #3572 ... a setting in the controlpanel enables showing the FocusTiddler with a thin border or alternative user-defined styles, but that uses CSS.escape

BurningTreeC added 4 commits November 28, 2018 12:07
@BurningTreeC BurningTreeC changed the title make navigator update "current-tiddler" field ... make navigator update "focus-tiddler" field ... Nov 28, 2018
@BurningTreeC
Copy link
Contributor Author

  • removed the stylesheet
  • made the story-startup set focus-tiddler, too

@BurningTreeC
Copy link
Contributor Author

I'm done here

@Jermolene Jermolene added the v5.1.20 Planned for v5.1.20 label Nov 29, 2018
@BurningTreeC
Copy link
Contributor Author

Hi @Jermolene , I think this would be a good feature to have. How do you want to proceed with this?

It would be useful if we could also access the previously navigated tiddler in a simple way, maybe through populating a navigation-history list

@BurningTreeC
Copy link
Contributor Author

Hi @Jermolene , I've resolved the merge conflict. What should we do with this?

@Jermolene
Copy link
Member

Hi @BurningTreeC there's another ticket where we've been talking about preserving the focus over the refresh cycle. Perhaps we should review that work before we merge this PR. The motivation being that if we had a robust way to preserve the focus then we should be able to use the browser focus mechanism to keep track of the currently selected tiddler.

@Jermolene
Copy link
Member

(I also realised that we probably shouldn't re-use $:/History for this, because it's nothing to do with navigation. If anything, the focus storage could be part of $:/StoryList).

@BurningTreeC
Copy link
Contributor Author

@Jermolene , I agree with both

I don't have this in mind for the focus-preserving within inputs
it's really more for navigating the story with keyboard shortcuts

this can allow to set focus to an input of the navigated tiddler in the end, too, but that's a (positive) side-effect

@Jermolene
Copy link
Member

I don't have this in mind for the focus-preserving within inputs it's really more for navigating the story with keyboard shortcuts

But if we had focus preservation then couldn't we use the browsers own focus handling to manage the current tiddler? (the current tiddler would be the one containing the focus, plus we'd make the tiddler frame be focusable).

@pmario
Copy link
Member

pmario commented Mar 12, 2019

IMO We have to keep in mind, that there can be several $:/StoryList's!!

@Jermolene
Copy link
Member

IMO We have to keep in mind, that there can be several $:/StoryList's!!

Indeed, I should have spelled out that I meant the tiddler named in the story attribute of the navigator widget.

@BurningTreeC
Copy link
Contributor Author

But if we had focus preservation then couldn't we use the browsers own focus handling to manage the current tiddler? (the current tiddler would be the one containing the focus, plus we'd make the tiddler frame be focusable).

that's a good point

@BurningTreeC
Copy link
Contributor Author

yes, the storylists have to be taken into account, you're right

@BurningTreeC BurningTreeC deleted the patch-61 branch June 23, 2019 13:30
@BurningTreeC BurningTreeC restored the patch-61 branch July 4, 2019 07:32
@BurningTreeC BurningTreeC reopened this Jul 4, 2019
@BurningTreeC
Copy link
Contributor Author

opening this again, because of #4061

@pmario
Copy link
Member

pmario commented Jul 4, 2019

@BurningTreeC

fix toc and prepare for link highlighting if element is selected #3975 will offer the same possibilities, without the need to modify navigator.

see the video: https://www.youtube.com/watch?v=iaevaODdKyA

The PR 3975 contains a stripped down version of the video. ... but it prepares everything that the focus-tiddler plugin will work in the future.

@BurningTreeC
Copy link
Contributor Author

closing in favor of #3975

@pmario
Copy link
Member

pmario commented Jul 4, 2019

In the future the "storyview.js" will need to be updated, because the storyview is the only instance, which can really tell which tiddler is "on-top" aka "focused"

@BurningTreeC
Copy link
Contributor Author

@pmario,

but it prepares everything that the focus-tiddler plugin will work in the future.

I'm not worried about the plugin... I'm just interested where the "viewed tiddler" is stored :)

@BurningTreeC BurningTreeC reopened this Jul 13, 2019
@BurningTreeC
Copy link
Contributor Author

@pmario I've reopened this PR

@Jermolene is it possible to have this (or similar) for 5.1.20?

@pmario
Copy link
Member

pmario commented Jul 13, 2019

@BurningTreeC ... This is way to complicated. ... The story.js has to do the job and it only needs to change 1 line of code and make it 2 for readability.

@BurningTreeC
Copy link
Contributor Author

@pmario

The story.js has to do the job and it only needs to change 1 line of code and make it 2 for readability.

do you PR?

@pmario
Copy link
Member

pmario commented Jul 13, 2019

I can try, but it was rejected, as included in the toc improvements.

@BurningTreeC
Copy link
Contributor Author

@pmario but it keeps track of closing, deleting and canceling, too

@@ -126,6 +126,16 @@ fromPageRect: page coordinates of the origin of the navigation
*/
NavigatorWidget.prototype.addToHistory = function(title,fromPageRect) {
this.wiki.addToHistory(title,fromPageRect,this.historyTitle);
this.wiki.setText(this.historyTitle,"focus-tiddler",undefined,title);
Copy link
Member

Choose a reason for hiding this comment

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

This probably causes an unneeded re-rendering of the story river, since you modify the history list 2 times. Modifying history list needs to be atomic write action

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify that the async nature of the refresh mechanism prevents that from happening: these two writes will just add the $:/HistoryList to the list of changes queued up for the next refresh, which actually occurs after an async timeout.

@BurningTreeC BurningTreeC reopened this Aug 3, 2019
@BurningTreeC
Copy link
Contributor Author

I'm reopening this because it would be great to have for 5.1.20

@BurningTreeC
Copy link
Contributor Author

closing in favor of a new, cleaner attempt

@BurningTreeC
Copy link
Contributor Author

see #4156

@BurningTreeC BurningTreeC deleted the patch-61 branch July 20, 2020 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v5.1.20 Planned for v5.1.20
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants