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

Context menu #342

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

br1tney5pear5
Copy link
Contributor

Context menu for cells. No concrete use case yet but I added 2 example entries: filter by the value in the current cell and open link. Shown on the gif below

context-menu

@liquidaty
Copy link
Owner

@br1tney5pear5 what key sequence triggers the context menu example in your above GIF?

@liquidaty
Copy link
Owner

@br1tney5pear5 could you add some basic tests? Please avoid using the ${EXPECT} test framework and instead adopt the same as or similar approach to test-sheet-1 defined in app/test/Makefile

@br1tney5pear5
Copy link
Contributor Author

br1tney5pear5 commented Dec 17, 2024

@liquidaty

what key sequence triggers the context menu example in your above GIF?

I mapped it to 'c' just for testing. It uses the same key mapping as sheet uses for up and down movement. Enter confirms, any other key cancels the menu.

could you add some basic tests? Please avoid using the ${EXPECT} test framework and instead adopt the same as or similar approach to test-sheet-1 defined in app/test/Makefile

I considered that, but since this menu is just a proof of concept, it should likely remain disabled for now. Therefore there isn't much to test at the moment. The goal here was to implement a generic context menu, and you'll notice that "Open Link..." is quite basic - it appears even if the cell doesn't parse as a URL, and it currently only works on Linux.

One of my upcoming tasks is to add two specific entries to this context menu. My suggestion is to disable the context menu for now and add the tests when I implement the requested context menu entries.

@liquidaty
Copy link
Owner

I considered that, but since this menu is just a proof of concept, it should likely remain disabled for now

Let's kill two birds with one stone:

  1. Make sure it has everything necessary to be used via extension, by adding a context menu to output of the pivot table in mysheet_extension.c
  2. Use this extension example as the basis for tests

You can test out the pivot, without the above, as follows:

> cd app/ext_example
> ./configure && make
...
Built /.../zsv/build/.../bin/zsvextmy.so
Built /.../zsv/build/.../bin/zsvextmysheet.so

> make -C .. build-cli 2>/dev/null
...
Built /.../zsv/build/.../bin/cli

> /.../zsv/build/.../bin/cli register mysheet
Loaded zsvextmysheet.so from /.../zsv/build/.../bin/zsvextmysheet.so
Loaded extension zsvextmysheet.so
Extension mysheet registered
Exiting mysheet extension example!

> make -C ../test worldcitiespop_mil.csv
...

>/.../build/.../bin/cli sheet ../test/worldcitiespop_mil.csv
image
# Type 'v', then 'country', then Enter
image image
# Scroll to any non-header roll and hit Enter to drill-down
image

This is the step where you could make a change such that on Enter, a menu is displayed, one of which is to do the above action, and the other which would do some other arbitrary action. These could be the test subjects.

(Note: there is a little extraneous stderr output in mysheet_extension.c that will be removed shortly, and would need to be removed before the test)

@br1tney5pear5
Copy link
Contributor Author

Hey, I've got an issue:

My tests involve registering extensions
Registering extensions is done with cli
cli isn't built if I just run make test, so it isn't available to tests
Furthermore zsv_sheet (as opposed cli sheet) doesn't seem to pick up extensions at all

I could look into it but I never worked with autotools much and it would probably be much easier for you to let me know the best way to go about it.

@liquidaty
Copy link
Owner

The easiest place to put these tests would probably be app/ext_example/Makefile. There you can see examples that use extensions with the CLI that invoke sheet (e.g. test-sheet-extension-1). Could you pls move your tests there, and if they still fail then we'll take a look as well?

@br1tney5pear5
Copy link
Contributor Author

Done that, Thanks.

Now the build for the mysheet extension is failing (I think it might've not been built by ci before) due to missing jq and utf8proc libraries. I tried adding libjq-dev and libutf8proc-dev to Dockerfile but that didn't work. I'm pretty weak when it comes to build stuff so will need some help here.

@br1tney5pear5
Copy link
Contributor Author

@liquidaty Hey, just following up in case you missed my last comment. I can try to figure out how to sort out the libraries myself but I'm not familiar with the ci pipeline so pretty sure that it'll be much quicker to do for you. Thanks.

@liquidaty
Copy link
Owner

Thanks, will take a look... but won't be able to do so for at least a week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants