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

URL Object Source #2408

Merged
merged 9 commits into from Jan 10, 2018
Merged

URL Object Source #2408

merged 9 commits into from Jan 10, 2018

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Nov 23, 2017

As discussed way back in #767, this adds some UI that will let you scan file contents from a remote URL.

One thing I can’t figure out is how to get the URL to “stick” when the text box loses focus. (I hate dealing with NIBs.) But other than that, it works.

There are some small bonus fixes at the end.

@skurfer skurfer changed the title Urlsource URL Object Source Nov 23, 2017
Copy link
Member

@tiennou tiennou left a comment

A few smallish comments. I'd just like the unrelated changes to fix #2409 in another PR, especially since the changes are small and can be merged pronto.

@@ -628,7 +628,9 @@ - (void)selectObject:(QSBasicObject *)obj {

- (void)objectIconModified:(NSNotification *)notif {
// icon changed - update it in the pane
[self setNeedsDisplay:YES];
QSGCDMainAsync(^{
Copy link
Member

@tiennou tiennou Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, I'd prefer whatever sent the notification to be where that main-async call is added, but yeah 😉.

@@ -2,6 +2,8 @@
#import <QSCore/QSClangAnalyzer.h>
#import "QSObject.h"

NSArray *recentDocumentsForBundle(NSString *bundleIdentifier);
Copy link
Member

@tiennou tiennou Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Namespacing pretty please ?

Copy link
Member Author

@skurfer skurfer Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean rename the function to something more unique?

Copy link
Member

@tiennou tiennou Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, just to be on the safe side.

if (![super settingsView]) {
[NSBundle loadNibNamed:NSStringFromClass([self class]) owner:self];
}
return [super settingsView];
Copy link
Member

@tiennou tiennou Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me wonder, but #2255 is still unmerged. Oh well.

Copy link
Member

@tiennou tiennou Dec 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was doubly wrong then, I meant #2256 🤣.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 28, 2017

I’ve moved the download status strings to another PR.

I’ve also changed the icon stuff, but I’d still like to talk about it…

When you post the notification, you don’t know who might pick it up and what thread they need to do the work on, so doesn’t it make more sense to let the observer handle that? Plus, for the sake of argument, let’s say objectIconModified: was being called from somewhere other than the notification. It needs to run on the main thread in that case too, but now there’s no assurance that it will.

NSMutableDictionary *settings = self.selectedEntry.sourceSettings;
if (!settings) {
settings = [NSMutableDictionary dictionaryWithCapacity:1];
[[self currentEntry] setObject:settings forKey:kItemSettings];
Copy link
Member Author

@skurfer skurfer Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of updated classes, I get a warning here about currentEntry of course, but I don’t see an alternative. QSCatalogEntry doesn’t ever initialize self.info[kItemSettings], and it doesn’t give us a way to do it either. Should we add a setter method to that class, or just do it in init?

Copy link
Member

@tiennou tiennou Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ought to be confused by #2255 not being in and having a bunch of expectations about how it should behave being wrong because it's not yet in. Hence, I don't really care if what you're doing is hacky, as the API is a mess. If this goes in before #2255, I'll just fix that code in the other PR (I will try to work on that soon™).

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 28, 2017

When you post the notification, you don’t know who might pick it up and what thread they need to do the work on.

Given the amount of bugs we had because of the "originating" thread being not obvious, I'm really tempted to explicitly make that API main-thread by default, instead of the hodge-podge it is now. I mean, everyone who choses to respond to that notification is very likely to 1) be trying to update its GUI in some way in response to the icon being loaded 2) cause a main-thread violation because it's not obvious you've just been pinged on the background loader thread/whatever else sent it. Hence the stance I have now w.r.t to this : "every posting of that notification MUST be made on the main thread".

Plus, for the sake of argument, let’s say objectIconModified: was being called from somewhere[…]

As per the rule above, I'd be tempted to say that it's "somewhere's" responsibility to make sure GUI calls are made on the main-thread. You could simplify out the whole notification out, and "somewhere", calling -setNeedsDisplay: from a background thread would still be incorrect without a main-thread dispatch.

Maybe I should have opened an issue to clarify, but that was why I started #2255 in the first place 😉 .

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 13, 2017

I’ve rebased this to take advantage of stuff in #2255 and renamed the “recent documents” function. Give it a look when you can.

On the icon notification…

everyone who choses to respond to that notification is very likely to 1) be trying to update its GUI in some way

Yes, in this case we know all the places it gets used and what it’s doing so it’s not the best example. But in general, you won’t always know.

Plus, for the sake of argument, let’s say objectIconModified: was being called from somewhere[…]

As per the rule above, I'd be tempted to say that it's "somewhere's" responsibility to make sure GUI calls are made on the main-thread.

But what if the GUI call is 4 methods deep into what you’re calling and you missed it because you didn’t follow every possible code path? What if the GUI call is conditional and only hit 2% of the time a method gets called?

@@ -981,7 +981,9 @@ - (void)setIcon:(NSImage *)newIcon {
[icon setCacheMode:NSImageCacheNever];
if (iconChange) {
// icon is being replaced, not set - notify UI
[[NSNotificationCenter defaultCenter] postNotificationName:QSObjectIconModified object:self];
Copy link
Member

@tiennou tiennou Dec 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: weird indent

@tiennou
Copy link
Member

@tiennou tiennou commented Dec 16, 2017

Apart from a nitpick, code looks good, haven't tested though (I'd be interested in your use-case 😉).

About that GUI-call discussion, I don't think you can convince me 😉. In the specific case of icons, I'd really prefer the notification be sent from the main thread because that's the most logical thing to do. I can think of a few places that use Cocoa bindings to listen to icon changes coughCatalog > Scanned objectscoughnever workedcough, which makes the fix above incomplete. If something ever wants that notification for non-GUI stuff, it's up to it to not lockup the main thread or async it somewhere else, but I'd expect this to be exceptional.

But what if the GUI call is 4 methods deep into what you’re calling and you missed it because you didn’t follow every possible code path? What if the GUI call is conditional and only hit 2% of the time a method gets called?

Just so we're clear, I consider fishy anything that call -someNotificationObserver:(NSNotification *) methods directly, so I'll usually take a closer look, extract a common method and ensure the observer behaves correctly w.r.t queues (or whatever). Granted, I might miss something, as it's true "in general you won't always know", but I think those few rules help to make that true.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 16, 2017

(I'd be interested in your use-case 😉).

Partly it was just a shame that all the pieces were there, but not hooked up.

You could use this to grab bookmarks from a remote site, index frequently used documentation, etc.

One use for me personally: We have dozens of Trac instances at work. I have script that generates an index of 3 links for each one (the main page, my tickets, and a search URL). Right now, I have to copy this file to my local machine periodically so it can get indexed by QS.

Just so we're clear, I consider fishy anything that call -someNotificationObserver:(NSNotification *) methods directly

OK, true. But even if such a method is getting called in response to a notification and not directly, it could have a UI update lurking 5 calls deep, or in a rarely triggered else clause.

@skurfer skurfer merged commit cd0fb41 into master Jan 10, 2018
2 checks passed
@skurfer skurfer deleted the urlsource branch Jan 10, 2018
skurfer added a commit that referenced this issue Jan 10, 2018
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.

None yet

2 participants