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

Better Bookmarking #28

Open
codythegreat opened this issue Oct 31, 2019 · 4 comments
Open

Better Bookmarking #28

codythegreat opened this issue Oct 31, 2019 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@codythegreat
Copy link
Owner

Better Bookmarking

Currently DSS supports bookmarking using an array to store slide - register pairs. This array is 5x2 in length.

I think that there could be a better solution for storing bookmarks. If anyone wants to theorize on this that would be great.

Hint: check out this ascii table http://www.asciitable.com/ notice that there are 128 potential values. Can this somehow be useful in our solution?

@codythegreat codythegreat added enhancement New feature or request good first issue Good for newcomers labels Oct 31, 2019
@amagura
Copy link
Contributor

amagura commented Nov 5, 2019

We could use "syntax" similar to Vim's marks: m starts a mark and a letter names it; so to create bookmark a, the user would type m + a. Backtick and apostrophe are used to jump to bookmarks: ` + a or ' + a.

@amagura
Copy link
Contributor

amagura commented Nov 5, 2019

I haven't looked at the current parsing code yet, but if we aren't loading the presentation into memory (as a single block of memory) we could use ftell to get the file offset of each slide and then use fseek to go the offset stored in the bookmark.

Bookmarks themselves should be implemented as a hash table where the bookmark name is the key and the file offset, pointer, or whatever is the value.

@codythegreat
Copy link
Owner Author

We could use "syntax" similar to Vim's marks: m starts a mark and a letter names it; so to create bookmark a, the user would type m + a. Backtick and apostrophe are used to jump to bookmarks: ` + a or ' + a.

Currently the program is set to use b and B to set a book mark and jump to bookmark. This is a departure from Vim's keybindings. I'm not sure if it'd be better to stick with Vim's bindings, but if there is a good reason to switch over then we can.

I haven't looked at the current parsing code yet, but if we aren't loading the presentation into memory (as a single block of memory) we could use ftell to get the file offset of each slide and then use fseek to go the offset stored in the bookmark.

Interesting. I need to look into these functions as they may come in handy in the future. As for DSS I don't think they'd be applicable as DSS loads the presentation into memory and closes the file. So for now it is just easier to jump to a slide by using the slides's number.

Bookmarks themselves should be implemented as a hash table where the bookmark name is the key and the file offset, pointer, or whatever is the value.

I worked on this issue this morning actually, and the solution I have implemented now changes the bookmark array from [5][2] to [128]. Now the index is the "character" and the value at the index is the slide number. You can see this in commit d890239

If you believe that this could be done better than what I did in the above commit please let me know and we can collaborate on a better solution. I'll leave this issue open for now.

side note: I just saw your comment on the buffer overflow issue. The solution for that was increasing the display's current slide counter to accommodate more digits. I don't like how that counter is implemented so I'm going to post an issue on it. But for now the buffer overflow issue is fixed.

@amagura
Copy link
Contributor

amagura commented Nov 5, 2019

So for now it is just easier to jump to a slide by using the slides's number.

An alternative approach would be to load each slide by memory address; then we could just store bookmarks as the memory address of whatever slide they point to.

If you believe that this could be done better than what I did in the above commit please let me know and we can collaborate on a better solution. I'll leave this issue open for now.

I think a more dynamic (memory) approach would be better, something growable, but it can happen later down the road.

side note: I just saw your comment on the buffer overflow issue. The solution for that was increasing the display's current slide counter to accommodate more digits. I don't like how that counter is implemented so I'm going to post an issue on it. But for now the buffer overflow issue is fixed.

This wednesday I'll catch my fork up with yours and work on replacing some of the static memory allocations with dynamic ones--where appropriate. I think the slideCount is already dynamic in my fork.

Edit: I won't be able to dig into furthering my fork until later this week, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants