random fixes and maybe improvements #147

koo5 · 2020-10-24T05:28:42Z

preliminary. have not gotten back to docker yet.

…ypes, but also commented out some log lines i wasnt interested in..

koo5 · 2020-10-25T14:03:33Z

so, one thing to discuss is the explicit "python3.9" everywhere. I should review if it's still needed or if it was ever really needed. The problem was with the indexer image, which pulled in multiple python versions. i'll continue later...

I changed some things like 'Not visited' to a more generic 'No data'. Best if i will outline my plans with this codebase to explain that..

karlicoss · 2020-10-26T00:12:55Z

Thanks! I'll take a look tomorrow.

upd: related issue, perhaps can close after the updated extension/backend are released #129

karlicoss

Thanks, left some comments!

src/promnesia/server.py

src/promnesia/sources/auto.py

karlicoss · 2020-10-26T21:09:42Z

src/promnesia/server.py

+            visits = [binder.from_row(row) for row in conn.execute(query)]
+        except sqlalchemy.exc.OperationalError as e:
+            if e.msg == 'no such table: visits':
+                return result


Hm, that would be misleading, e.g. when you have some error, you'd get zero visits in the backend. Wondering what would be your usecase?
One thing I can think of is when you're using the extension and the backend isn't working, you don't get 'local' visits either. I guess the right way to handle this would be to support displaying both errors and valid results at the same time in sidebar.

hmm, displaying results + errors/other messages would definitely be a move in the right direction. I'll see if the codebase can handle that without too much hassle.
My usecase was very dubious - eliminating exception traces from server log. See, server starts up, but until indexer first runs, you're getting these exceptions that the database doesn't exist. Took me a while to conclude that server doesn't have any "init empty" db code, that it's just waiting on the indexer to do it.

src/promnesia/server.py

extension/src/background.js

extension/src/display.js

karlicoss · 2020-10-26T21:17:50Z

extension/webpack.config.js

@@ -65,7 +65,7 @@ const commandsExtra = {
 // TODO ugh it's getting messy...
 const action = {
    "default_icon": "images/ic_not_visited_48.png",
-    "default_title": "Was not visited",
+    "default_title": "Show promnesia sidebar",


Maybe "Not visited (click to show the sidebar)"?

good compromise lol! But i wasn't trying to sneak in the "click to show sidebar", rather, i was trying to get rid of the "not visited", which i find misleading:

i will use the sidebar to alert me that maybe my friend wrote some notes about the url. Sure, he surely visited it first, but that's a bit of a stretch. What i want the sidebar to display is more general than "visits", it's any useful information.

if promnesia is having an issue with the database, it doesn't mean the page wasn't visited. Likewise, if i just installed the extension, it will show "not visited", but again that may not be true.

Then again, this was just a bit of bikeshedding to warm me up to the codebase.

Ah I see!

Yeah, although would need to rethink what information to communicate in the icon anyway

Yep, but ideally you set up the database and it doesn't have issues anymore after that. So worrying about the text when the DB isn't working is the least of your concerns :)

But I guess either way I'm pretty indifferent to the title, because it's only useful the first few times you use the extension, then you just remember which icon is which.

karlicoss · 2020-10-26T21:26:51Z

src/promnesia/common.py

@@ -40,7 +40,16 @@ def file(cls, path: PathIsh, line: Optional[int]=None, relative_to: Optional[Pat
        ll = '' if line is None else f':{line}'
        # todo loc should be url encoded? dunno.
        # or use line=? eh. I don't know. Just ask in issues.
-        handler = _detect_mime_handler()
+
+        # todo: handler has to be overridable by config. This is needed for docker, but also for a "as a service" install, where the sources would be available on some remote webserver


hm, not sure what you mean here? If you install https://github.com/karlicoss/open-in-editor in Docker it's definitely not going to work either way?
Or do you mean that paths mounted in docker won't work on the host system? I generally just use the same paths when using Docker, makes everything much easier.

Right, i mean that paths will be relative to the docker root. Using the same paths sounds like a nice trick, but still. And, running promnesia on a server, i want to prepend some "http://my-server/path/to/data/", and ensure that ":line" is not appended. Well, this makes me think that for a multi-user setup, we could eventually make it a setting in the client. But that's a long way ahead

I think ideally you'd be allowed to add a bit of Javascript in the settings, as a hook, so you'd be able to hack arbitrary sidebar data, including the paths. This is nice to have anyway, to allow experimenting/fine tuning without building the extension yourself. And I feel like a setting to prepend the paths is a bit too specific anyway.

But in the meantime, did it help you to use the same paths in docker as on the host system? If yes can perhaps remove MIME_HANDLER stuff from the other files?

eval()'ed hooks sound like a good way to get some rudimentary customizability going, yeah.
But here i'm talking a usecase of getting my clients and coworkers and friends (yes, all of them) to install this extension. The most i can ask them is to install it, go to settings and set the server to <url> and click save, or other similarly basic sequence of actions. It could possibly be `install it, go to settings and insert into "hooks list" or something like that.

But it's the server that knows where it stores data and how people can access them, so i don't see how it shouldnt' be the server that gives this info to the client. Not necessarily by composing a fixed "href" as it does now. Sooner or later the client will need to be smarter, so we may as well start giving it data in a more semantic way: pass is the path, line number, and a suggested pattern to put this together, maybe?

Maybe you'd rather like to see these responsibilities split between the indexer and the "server"? That'd make sense to me.

use the same paths in docker as on the host system

not quite the robust solution i'm looking for

setting to prepend the paths is a bit too specific anyway.

yeah.. but hey, it will get the job done

whatever design that will be aligned with your mental model though!

not quite the robust solution i'm looking for

Yeah, agree it's kind of unsatisfactory. Although in general I've had much less grief with Docker since I started doing this, but that's just me :) P.S. also just realized that you not only wanted to patch a local path, but to point it to your server, so my suggestion wouldn't work anyway.

yeah.. but hey, it will get the job done

Yep, generally I'm all for it, but I'm trying to be cautions with adding items to the config, because it's kind of a promise to keep it backwards compatible.

Otherwise, agreed, makes sense! Especially the point that it doesn't seem like frontend's responsibility.

I guess a way that would make more sense without any extra code changes is running an sqlite query right after indexing to 'fix' the paths? Still kinda hacky, but you only need to do this once and it's very flexible. But my concern with that would be that eventually I want to support realtime indexing, and this wouldn't quite work.

Another, nicer but stil not too specific option would be a config hook, through which all visits pass before getting written in the database? E.g. if I understood your usecase, it would look like:

# in config.py def hook(v: Visit): # this will have a nicer name? href = v.locator.href or "" if "editor://" not in href: return # it's fine, not need to patch href = href.replace("editor://", "http://your-serlver") href = re.replace(r":.*", "", href) # chop off line number v.locator.href = href return v

The upside is that it will be quite flexible and potentially support usecases we couldn't even anticipate. I even wouldn't mind merging specific 'hooks' in the repository, so they could be imported and used in config.py. What do you think?

koo5 · 2020-10-27T03:05:38Z

i need to go through all the changes and revert some nonsensical ones,...

koo5 · 2020-10-28T03:26:21Z

uff, this github UI is getting confusing, so i'll just dump my responses here like this:

using your own build of the extension is hardly a 'quick start'

good point, fixed wording.

how to run CI for the pull requests

at least it should let you fire a http hook. I can look into it sometime

not that I expect your changes would break anything

now that's a good one! Indeed, mypy was complaining.
scripts/ci/extension finishes with:

Your web extension is ready: /home/koom/promnesia/extension/dist/chrome/web-ext-artifacts/promnesia-0.11.14.zip

exit 1

, does that sound about right?

const LOCAL_TAG = 'this_browser_history';

I understand that lenght matters. But i don't want to configure it for myself, i want it to be meaningful to people who just installed the extension and are trying to understand what's going on. "local" is confusing, it could mean "local to my city" or anything. It was confusing for me. So, lol, maybe i should suggest, audaciously, that you configure it to something short for yourself, or that we configure it to something as descriptive as "this_browser_history" for new users?

tabId:' + tabId + ' url: ' +

sure, why not

(so the users can experiment/work around for themselves).

maybe...that's more of a developer thing maybe. Especially if setting up a build env for the extension is fairly trivial

Hmm, not sure about moving this file..

got it!

koo5 · 2020-10-28T03:28:55Z

phew, isn't this exhausting?

koo5 · 2020-10-28T16:28:40Z

trying to be cautions with adding items to the config, because it's kind of a promise to keep it backwards compatible.

i'm trying to add MIME_HANDLER, you're proposing to add def hook(v: Visit):. Neither seems to have any effect on currently existing configs, unless they happen to use those identifiers already. Can you explain what you mean by backwards compatibility here?

karlicoss · 2020-10-28T21:05:24Z

scripts/ci/extension finishes with exit 1

Hm, it shouldn't, means that one of the checkers failed. (most likely, npm run flow). But if you don't see any errors, I'll double check before merging.

"local" is confusing

Hm, ok, fair as well. Maybe "this_browser" then? Short enough, and "_history" feels redundant because it's all history in the sidebar (and later it would be good to fix it so tags support whitespace)

Can you explain what you mean by backwards compatibility here?

I mean that if we add MIME_HANDLER, it would be in the config, and then when doing some changes later, we'd need to be careful about not breaking it by accident, it's not specifically about your suggestion, but in general, the more stuff you have in the config, the harder it is to change software. In addition I feel like if MIME_HANDLER is just a simple string, it wouldn't be powerful enough and some other users would run into needing something more flexible (so we'd end up duplicating the efforts). Whereas the hook I'm suggesting is generic enough to justify the compatibility + it's potentially very flexible even for scenarios we don't anticipate now.

koo5 · 2020-10-29T01:00:47Z

"this_browser"

I kind of like it, but i'd end up trying to sneak in a second tag: "history". Really what we have here is a paradigm clash. To you, everything is a visit. To me, it's more about notes or comments, and generically about any info related to the page/subject. I see history as just a part of it. Let me revert this, and let's think how to accomodate the different perspectives for a while.

feel like if MIME_HANDLER is just a simple string, it wouldn't be powerful enough

ok, in general i agree with the "python over config files" paradigm.
But let's take a step back. Is it the indexer, that should know how a person with a browser can access the file? No. In other words, if 'href' is how the file/source is reachable, then 'href' should not be in the database at all. If 'href' is also how the file/source was originally reached ( that is, the address of the page that had the link that you clicked), that should be in the database. But we need to unconflate the terms.

Then, the server can give hints to the client. And i guess we agreed that the client should have the final word? Then it's the client that should paste everything together with some pattern. If you have to chop strings somewhere, you probably shouldn't be pasting them together earlier.

And then eventually, somebody's gonna come along who will want to run the indexer on another machine, and store the access details in the db...:) That's the kind of gotcha that's easy to handle if you represent things in rdf in the first place. But that's for another day..

Anyway, i don't think either of us wants to ponder this architectural stuff right now?

koo5 · 2020-10-29T17:56:25Z

note to self: there is a usecase to specify the href hook/pattern per-source. Say i have https://github.com/koo5/notes/ cloned locally, and index that, but i prefer the link to be right back to github

karlicoss · 2020-10-30T15:08:56Z

Really what we have here is a paradigm clash.

Ah ok I see what you mean here! Another option is perhaps let the local tag just be the browser name? E.g. you'd have firefox/chrome/etc there, and then it would be obvious that it's browser history?

But we need to unconflate the terms.

Yeah, so the promnesia term for the link you're viewing is 'url', and for the "how is the source rechable" is 'locator'.

But let's take a step back. Is it the indexer, that should know how a person with a browser can access the file? No. In other words, if 'href' is how the file/source is reachable, then 'href' should not be in the database at all.

Not sure what do you mean? E.g. if you indexed some text file and extracted some links from it... How would the client know which file is it coming from?

If you have to chop strings somewhere, you probably shouldn't be pasting them together earlier.

Hm, not sure if I got you right, but basically you suggest that maybe the server should have a way of returning the 'file location' structure directly, and then the client decides how to open it? E.g. this editor:// thing wouldn't be hardcoded in the indexer code? If yes, then I kinda agree that it makes sense, yes. I guess the reason I implemented it as it is because realistically you only have either 'normal' urls that you can open in your browser tab, or these mime things that can be used to open regular files. But maybe I haven't foreseen more 'types' you have in mind?

note to self: there is a usecase to specify the href hook/pattern per-source.

Yep, I also had similar thoughts!

karlicoss · 2020-10-30T18:22:45Z

Eh? not sure how that Ubuntu thing has slipped in the contributors.. Anyway, thanks @koo5 much appreciated!

koo5 · 2020-10-31T02:54:09Z

"ubuntu" is my misconfigured git on a vps lol. I remembered though to thank @etopeter, because some of the docker stuff in this PR i originally took from one of his unmerged PRs or something like that. So thanks, @etopeter .

koo5 · 2020-10-31T03:56:56Z

a pondering for a rainy sunday:

What i want to convey is this pattern that each component just does it's best in the sense that it puts up whatever it knows, for others to use or ignore. And the data keeps its structure, obviously. And then the user-overridable function makes the final decision. I think this pattern makes it easy to debug and extend things. Otoh, it can be confusing for the user. It has to be explained why there can be apparently conflicting informations. This could be solved with some more verbosity...


If Source is configured with browser_locator_href_pattern pattern, it sets browser_locator_href_pattern for each "visit".
	for example: Source(
		auto,
		'/path/to/cloned/repo/',
		browser_locator_href_pattern='http://github.com/user/repo/blob/master/{path}')

Indexer sets source_file_path to the absolute file path it sees, or sets source_url to the url it fetches the source from.

If Server detects xdg-open, it sets file_path_handler_protocol to 'editor'. Otherwise, maybe sets it to 'file'? (i don't bother thinking about the emacs handler since it's deprecated).

Server can be generically configured with a "server_configuration" object that it passes to the client.
	SERVER_CONFIGURATION = {'outside_path':'/home/koom/promnesia/docker/'}

====

so, to re-hash, the client can get:
	browser_locator_href_pattern from:
		Source
		Server
	file_path_handler_protocol from:
		Server
	source_file_path from:
		Indexer
	source_uri from:
		Indexer
	server_configuration from:
		Server
	line from:
		Indexer

now it's the client's job to construct a clickable href:
	if (source_uri)
		return source_uri
	if (server_configuration.outside_path)
		path = server_configuration.outside_path + '/' + source_file_path
	else
		path = source_file_path
	if (browser_locator_href_pattern)
		return eval(browser_locator_href_pattern)
	if (file_path_handler_protocol == 'editor')
		return eval("editor://\{path\}:\{line\}")
	return eval("file://\{path\}")

well, ideally, this function should be explicit about what's in scope for these evals, etc. This is just a rough idea.

@koo5

thanks @koo5 for spotting the flaky test (#147)

@koo5

spotted by @koo5 #147

@koo5