-
Notifications
You must be signed in to change notification settings - Fork 74
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
random fixes and maybe improvements #147
Conversation
…ypes, but also commented out some log lines i wasnt interested in..
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.. |
Thanks! I'll take a look tomorrow. upd: related issue, perhaps can close after the updated extension/backend are released #129 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, left some comments!
src/promnesia/server.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Not visited (click to show the sidebar)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting to prepend the paths is a bit too specific anyway.
yeah.. but hey, it will get the job done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whatever design that will be aligned with your mental model though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
i need to go through all the changes and revert some nonsensical ones,... |
uff, this github UI is getting confusing, so i'll just dump my responses here like this:
good point, fixed wording.
at least it should let you fire a http hook. I can look into it sometime
now that's a good one! Indeed, mypy was complaining.
, does that sound about right?
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?
sure, why not
maybe...that's more of a developer thing maybe. Especially if setting up a build env for the extension is fairly trivial
got it! |
phew, isn't this exhausting? |
i'm trying to add |
Hm, it shouldn't, means that one of the checkers failed. (most likely,
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)
I mean that if we add |
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.
ok, in general i agree with the "python over config files" paradigm. 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? |
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 |
Ah ok I see what you mean here! Another option is perhaps let the
Yeah, so the promnesia term for the link you're viewing is 'url', and for the "how is the source rechable" is 'locator'.
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?
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
Yep, I also had similar thoughts! |
Eh? not sure how that Ubuntu thing has slipped in the contributors.. Anyway, thanks @koo5 much appreciated! |
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...
|
preliminary. have not gotten back to docker yet.