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

Add submit menu to send data to server #7

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

Grandro
Copy link
Collaborator

@Grandro Grandro commented Oct 9, 2023

This PR adds a basic submit menu GUI to send data to the server. This lets the frontend be functionable without having to specify all necessary parameters in the URL and is more user-friendly. In case all parameters are properly specified, the menu won't show up until manually requested by a dedicated button at the top. It is not possible to send data while a request is already being processed. The tab to send GeoJSON data is not functionable yet.

I restructured index.html and introduced the export button look as the default look of a button.
I also changed the third argument when calling curl_easy_getinfo with CURLINFO_RESPONSE_CODE to be of datatype long instead of int because the docs suggest this change and it fixed a segment fault for me when using the Debug build variant of CMake.

@patrickbr
Copy link
Member

patrickbr commented Oct 18, 2023

Thank you for that! I was wondering how we could achieve consistency between the query editor in your menu and the QLever UI, in particular regarding autocompletion and syntax highlighting. I think as a first step it should be easy to use a full resizable text box for query editing, and add some syntax highlight. But we also need a button "Edit in QLeverUI" which redirects to the QLeverUI filled with the query.

I think the GeoJSON upload functionality is an extremely useful idea. Do you have a plan how to build this in the backend? I expect the easiest way would be to just add the possibility to fill a GeomCache via a GeoJSON string / stream.

I also changed the third argument when calling curl_easy_getinfo with CURLINFO_RESPONSE_CODE to be of datatype long instead of int because the docs suggest this change and it fixed a segment fault for me when using the Debug build variant of CMake.

Ha, we seem to have fixed this bug independently on the same day (8359721) :) I spent an entire day searching for the root cause of the corresponding segfault... it occured on non-debug builds on my machine when compiled with clang, but I wasn't able to reproduce / locate it in gdb, with valgrind or with the address sanitizer. I finally resorted to commenting out lines until the segfault went away. How did you find the cause?

@Grandro
Copy link
Collaborator Author

Grandro commented Oct 18, 2023

Ha, we seem to have fixed this bug independently on the same day (8359721) :) I spent an entire day searching for the root cause of the corresponding segfault... it occured on non-debug builds on my machine when compiled with clang, but I wasn't able to reproduce / locate it in gdb, with valgrind or with the address sanitizer. I finally resorted to commenting out lines until the segfault went away. How did you find the cause?

I'm sorry to hear we both wasted a lot of time on this ^^' Funnily for me it only occured when using the Debug build... Before I always used RelWithDebInfo which worked fine, not sure about the others. I was trying to fix another bug and wanted to use the Debug build type for that because I wanted to use breakpoints but felt like too many optimizations were done which made them kind of useless. I remembered I already told you about this issue in a mail and gave it another try to find a fix. I then tried to set breakpoints later and later up until to the point where the segfault happened and realized that if I comment out the call to GeomCache::requestIndexHash and just used an empty string there was no segfault. Because there is a lot of stuff with curl I started to go through the docs for every function call because I had no idea how anything worked and then stumbled across this difference by chance.

Edit:

I think the GeoJSON upload functionality is an extremely useful idea. Do you have a plan how to build this in the backend? I expect the easiest way would be to just add the possibility to fill a GeomCache via a GeoJSON string / stream.

Yes, my idea was to URI encode the content of the .geojson file, send the encoded data to the server which then parses the data and then fill the GeomCache with that.

@patrickbr
Copy link
Member

patrickbr commented Jun 19, 2024

Thanks! I did some preparation for merging this into master, in particular I merged master (which now includes the updated loading bar) into submit-menu in a new branch here: https://github.com/ad-freiburg/qlever-petrimaps/tree/Grandro-submit-menu

@Grandro, can you check whether this branch works as expected? If you find anything and want to fix it, please fork https://github.com/ad-freiburg/qlever-petrimaps/tree/Grandro-submit-menu and start from there.

I also noticed that the GeoJSON upload does not support multigeometries and polygons with holes yet. Are you aware of this problem?

@Grandro
Copy link
Collaborator Author

Grandro commented Jun 19, 2024

Thanks, I will check if it works as expected.
No, I was not aware that multigeometries and polygons with holes are not supported yet, sorry that I missed that.
image
image
image
image
Are these the ones you mean? I will look into implementing the missed support in the branch you created.

@patrickbr
Copy link
Member

Yes, these are the ones I meant. The existing SPARQL-GeometryCache implements these as follows: multigeometries are stored consecutively in the underlying array, and only the first has a "standard" QLever-ID. The following multigeometries have a special placeholder-ID, so if e.g. geometry 345434 is requested, a lookup is made in the array holding all jump marks into the points array, and if the next entry after this entry has such a placeholder QLever-ID, it is also used to construct the return geometry, and so on. You also have such an array in your GeoJSONGeomCache class which currently only holds the jump offset into the points vector, so I think all you have to do is to store pairs {offset, type}, where type is either 0 (normal), or 1 (continuation multi geometry) and handle this in the places where you build output geometries.

@Grandro
Copy link
Collaborator Author

Grandro commented Jun 20, 2024

I am not sure if I understand your explanation correctly. To my understanding the current implementation with the example of a MultiPoint pushes a point to _points for every coordinate in the array provided by the GeoJson but then handles the whole MultiPoint as 1 geometry (that is: increases _curUniqueGeom and stores the attributes/properties). But because internally when you request the objects with GeoJSONCache::getRelObjects there is an "object" created for each entry in _points, which leads to a crash if you then click on a point geometry of that MultiPoint which is not the one the attributes/properties were saved for.
So there are 2 possibilities to fix this:

  1. Have additional logic that handles which points in _points and which lines in _lines belong to a single and same geometry (I'm not sure if thats what you mean with the 'array in your "GeoJSONGeomCache"') and then only create an object for each set of points or lines.
  2. Treat each "sub-geometry" in a multigeometry as a unique geometry, e.g. each point in a MultiPoint as if it was a Point geometry. I believe this approach has the downside that when you then click on a geometry that would belong to a multigeometry, only that geometry can be highlighted orange because the connection to the others is lost. Is that unwanted behaviour?

I have pushed a commit that implements 2), if you want me to change it to 1) instead it would be helpful for me if you could provide variable names for already existing datastructures that you are referring to.

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