Issue 3241 - Make filter list URL configurable in devbuilds #30

dedecej · 2016-03-14T13:59:43Z

Related to Issue 3241.

Merge with Eyeo upstream

Sync with Eyeo

fhd · 2016-03-31T08:52:15Z

AdblockPlusSafari.xcodeproj/project.pbxproj

@@ -804,6 +845,36 @@
 			shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-AdblockPlusSafariTests/Pods-AdblockPlusSafariTests-resources.sh\"\n";
 			showEnvVarsInLog = 0;
 		};
+		83C19D903DD1D9F927CBD253 /* Copy Pods Resources */ = {


So, how I interpret this is, XCode got upgraded in the mean time and there are some pods related changes happening because of that? Could we put these in a separate commit? No need for an issue, just something like: "Noissue - XCode project file updates" would do. Then I can merge that one first.

Huh? I might understand what you asking but I am afraid that I am not able to split this commit to two standalone working parts. I can move xcode project file to different commit, and append this commit to this pull request. In that case you would have to pull both commits to be able to build it properly.

That would suffice, ideally it'd be a separate pull request.

fhd · 2016-04-15T15:31:27Z

@dedecej Did you notice my comment here?

dedecej · 2016-04-18T13:23:58Z

AdblockPlusSafari/AdblockPlus.m

+        @"easylist+exceptionrules_content_blocker.json"
+      };
+
+    for (NSString *defaultFilterListName in defaultFilterListsFileNames) {


I decided, that it would be more convenient to map filter lists by names rather then urls (which can be changed).

A couple of doc lines explaining the migration transformation would be handy

Sync with Eyeo

fhd · 2016-06-30T16:21:44Z

AdblockPlusSafariExtension/AdblockPlus+Extension.m


 @implementation AdblockPlus (Extension)

 - (NSURL *__nullable)activeFilterListsURL
 {
-  NSString *fileName;
+  NSString *filename;


Why the rename, isn't fileName how we normally capitalise this in the code base? Assuming Apple's coding style doesn't cover this case, I suggest we stay consistent with the rest of the project.

Fixed, I probably forgot about it.

fhd · 2016-06-30T16:26:39Z

Alright, starting on this one. Had a quick look and left some superficial comments. Will take a closer look tomorrow.

Sync with Eyeo

dedecej · 2016-07-11T13:28:50Z

@fhd: Rebased and updated.

pavel-zdenek

The only bigger issue is the hardcoded tableviewdatasource index shifting magic. And maybe the asserts.

pavel-zdenek · 2016-10-07T14:38:22Z

AdblockPlusSafari/AdblockPlusController.m

@@ -64,7 +66,7 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N
 {
  if ([keyPath isEqualToString:NSStringFromSelector(@selector(enabled))]) {
    self.blockingEnablingSwitch.on = self.adblockPlus.enabled;
-    [self.tableView reloadSections:[NSIndexSet indexSetWithIndex:1] withRowAnimation:UITableViewRowAnimationNone];
+    [self.tableView reloadSections:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, 2)] withRowAnimation:UITableViewRowAnimationNone];


An unfortunate binding of momentary design to hardcoded numbers. Someone will forget to update this when/if the design changes around. Like when a new section is inserted between. If such inserted section is NOT to be disabled on ABP toggle, then Range will be a wrong tool altogether. Any idea of how to make this programmatically safe?

I tried to do something, but unfortunately I cannot do more. It is possible to append any information to section in designer.

Well this is basically Massive View Controller inception, so wrapping the index arithmetic away in a separate class would be good. The pluses and minuses should not be scattered across the controller. But this is already 50% better than the original hardcode, so let's do the other 50% when there is another UI requirement.

pavel-zdenek · 2016-10-07T14:41:52Z

AdblockPlusSafari/AdblockPlusController.m

+
+- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section
+{
+  return [super tableView:tableView numberOfRowsInSection:section] + (section == 0 ? 1 : 0);


Hardcoded design again. section == 0 should be at least a declared constant "which section needs the section offseting magic".

pavel-zdenek · 2016-10-07T14:54:37Z

AdblockPlusSafari/AdblockPlusController.m

+      cell.textLabel.enabled = enabled;
+      return cell;
+    } else {
+      return [super tableView:tableView cellForRowAtIndexPath:[NSIndexPath indexPathForRow:indexPath.row - 1 inSection:0]];


Why not using correctedIndexPath here?

pavel-zdenek · 2016-10-10T08:39:28Z

AdblockPlusSafari/FilterList.h

 @property (nonatomic, strong, nullable) NSString *version;

 @property (nonatomic) BOOL userTriggered;
 @property (nonatomic) BOOL downloaded;
 @property (nonatomic) BOOL updating;
 @property (nonatomic) BOOL lastUpdateFailed;
 @property (nonatomic) NSUInteger taskIdentifier;
+@property (nonatomic) NSUInteger updatingGroupId;


As a clueless novice to the ABP code, i would like to have one line explanation of what is this number for. Task identifier would be nice to have explained too.

pavel-zdenek · 2016-10-10T08:43:33Z

AdblockPlusSafari/FilterList.m

  return self;
 }

 - (NSDictionary *)dictionary
 {
+  NSAssert(self.fileName, @"Filename must be set!");


Assert will do nothing on release run. Is this really just a debug sanity check? Are the asserts always true on release? If not, what will the function produce?

This should not happen. FilterList is create only from special dictionary, and that dictionary is not accessed from outside.

pavel-zdenek · 2016-10-10T08:56:14Z

AdblockPlusSafariExtension/AdblockPlus+Extension.m

    }
  }

+  if (!fileName) {
+    fileName = @"empty.json";


Should be declared constant. It is used in multiple places.

pavel-zdenek · 2016-10-17T14:46:54Z

@fhd LGTM

fhd

Finally managed to take a closer look. Looks good, some minor comments.

fhd · 2016-10-21T18:18:38Z

AdblockPlusSafari/AdblockPlus.m

+
+        for (NSString *defaultFilterListName in defaultFilterListsFileNames) {
+          NSString *defaultFilterListFileName = defaultFilterListsFileNames[defaultFilterListName];
+          for (NSString *filterListName in filterListsVersion1) {


How I see it, filterListName actually contains the filter list URL - so how about filterListUrl?

fhd · 2016-10-21T18:18:49Z

AdblockPlusSafari/AdblockPlus.m

+          for (NSString *filterListName in filterListsVersion1) {
+            NSDictionary *filterList = filterListsVersion1[filterListName];
+            if ([filterList[@"filename"] isEqualToString:defaultFilterListFileName]) {
+              NSMutableDictionary *modifiedFilterList = [filterList mutableCopy];


Why copy filterList and then modify it? Looks to me as if it would be cleaner and less code if we create a fresh object. Or am I missing something?

In this case I need to copy all properties from old filter list

fhd · 2016-10-21T18:46:58Z

AdblockPlusSafari/AdblockPlus.m

+        _filterLists = filterListsVersion2;
+
+      } else {
+        // No filter lists was loaded, use default filter lists


I guess we don't need an empty block just for this comment.

fhd · 2016-10-21T19:33:34Z

AdblockPlusSafari/AdblockPlusController.m

+{
+  if (section == AdblockPlusControllerSectionDefault) {
+    return [super tableView:tableView numberOfRowsInSection:section] + 1;
+  } else {


Nit: Another else after return, see above.

fhd · 2016-10-21T19:34:15Z

AdblockPlusSafari/CustomFilterListsController.m

+    NSDate *lastUpdate = [self.adblockPlus.filterLists[self.adblockPlus.activeFilterListName] lastUpdate];
+    if (lastUpdate) {
+      return [NSString stringWithFormat:defaultTitle, [self.dateFormatter stringFromDate:lastUpdate]];
+    } else {


Nit: Another else after return, see above.

fhd · 2016-10-21T19:34:43Z

AdblockPlusSafari/CustomFilterListsController.m

+  NSString *url = [NSUserDefaults.standardUserDefaults stringForKey:customFilterListKey];
+  if ([url length] == 0) {
+    return customFilterListUrl;
+  } else {


Nit: Another else after return, see above.

fhd · 2016-10-21T19:36:08Z

AdblockPlusSafari/CustomFilterListsController.m

+
+- (NSAttributedString *)footerTitleForFilterList:(FilterList *)filterList
+{
+  if (filterList) {


This block has a few else after return - I think the code can be simplified a bit.

fhd · 2016-10-21T19:36:46Z

AdblockPlusSafari/AdblockPlus.m

+{
+  if (!self.defaultFilterListEnabled && [self.filterLists[CustomFilterListName] downloaded]) {
+    return CustomFilterListName;
+  } else if (self.acceptableAdsEnabled) {


Nit: We typically avoid else after return - it's redundant. But I wouldn't insist. Same goes for the other occurrences I pointed out.

fhd · 2016-10-21T19:37:41Z

AdblockPlusSafari/AdblockPlusController.m

+
+- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
+{
+  if (indexPath.section == AdblockPlusControllerSectionDefault && indexPath.row > 0) {


This block has a few else after return - I think the code can be simplified a bit.

fhd · 2016-10-21T19:43:41Z

AdblockPlusSafari/AppDelegate.m


-          updated = updated || (!lastUpdate && currentLastUpdate) || (currentLastUpdate && [currentLastUpdate compare:lastUpdate] == NSOrderedDescending);
+          updated = updated || (!lastUpdate && !!currentLastUpdate) || (!!currentLastUpdate && [currentLastUpdate compare:lastUpdate] == NSOrderedDescending);


Why add the !! here? I don't think it changes the behaviour of this expression.

fhd · 2016-10-28T10:21:46Z

LGTM, I'll merge this.

fhd · 2016-10-28T10:25:07Z

Merged, thanks! d468b47

dedecej added 16 commits September 14, 2015 14:51

Merge pull request #1 from adblockplus/master

3f9818f

Merge with Eyeo upstream

Merge pull request #2 from adblockplus/master

883dbba

Sync with Eyeo

Merge pull request #6 from adblockplus/master

8dd31b1

Sync with Eyeo

Merge pull request #8 from adblockplus/master

2b6ddb2

Sync with Eyeo

Merge pull request #10 from adblockplus/master

5d327d0

Sync with Eyeo

Merge pull request #11 from adblockplus/master

f981501

Sync with Eyeo

Merge pull request #12 from adblockplus/master

1bfcc17

Sync with Eyeo

Merge pull request #13 from adblockplus/master

0fabdbd

Sync with Eyeo

Merge pull request #14 from adblockplus/master

a401f33

Sync with Eyeo

Merge pull request #15 from adblockplus/master

fe865e6

Sync with Eyeo

Merge pull request #16 from adblockplus/master

0cfeab5

Sync with Eyeo

Merge pull request #18 from adblockplus/master

2e639a2

Sync with Eyeo

Merge pull request #19 from adblockplus/master

fa25e77

Sync with Eyeo

Merge pull request #20 from adblockplus/master

8b168e5

Sync with Eyeo

Merge pull request #21 from adblockplus/master

3594515

Sync with Eyeo

Merge pull request #22 from adblockplus/master

0779676

Sync with Eyeo

fhd reviewed Mar 31, 2016
View reviewed changes

dedecej reviewed Apr 18, 2016
View reviewed changes

dedecej force-pushed the issue-3241 branch from 7652453 to aa45a22 Compare April 18, 2016 13:55

Merge pull request #23 from adblockplus/master

3d5e873

Sync with Eyeo

dedecej force-pushed the issue-3241 branch from aa45a22 to a5a7981 Compare May 6, 2016 14:17

Merge pull request #24 from adblockplus/master

2816cfe

Sync with Eyeo

fhd reviewed Jun 30, 2016
View reviewed changes

fhd self-assigned this Jun 30, 2016

Merge pull request #25 from adblockplus/master

b0e5466

Sync with Eyeo

dedecej force-pushed the issue-3241 branch 2 times, most recently from 337d821 to 797b501 Compare July 11, 2016 13:26

pavel-zdenek suggested changes Oct 10, 2016

View reviewed changes

dedecej force-pushed the issue-3241 branch from 797b501 to 5ae884c Compare October 17, 2016 13:40

dedecej force-pushed the issue-3241 branch 2 times, most recently from 7ac44ee to 9339f46 Compare October 21, 2016 08:24

fhd suggested changes Oct 21, 2016

View reviewed changes

dedecej force-pushed the issue-3241 branch from 9339f46 to b2eb308 Compare October 26, 2016 12:13

Issue 3241 - Make filter list URL configurable in devbuilds

3393125

dedecej force-pushed the issue-3241 branch from b2eb308 to 3393125 Compare October 26, 2016 12:18

fhd approved these changes Oct 28, 2016

View reviewed changes

fhd closed this Oct 28, 2016


		updated = updated \|\| (!lastUpdate && currentLastUpdate) \|\| (currentLastUpdate && [currentLastUpdate compare:lastUpdate] == NSOrderedDescending);
		updated = updated \|\| (!lastUpdate && !!currentLastUpdate) \|\| (!!currentLastUpdate && [currentLastUpdate compare:lastUpdate] == NSOrderedDescending);

Issue 3241 - Make filter list URL configurable in devbuilds #30

Issue 3241 - Make filter list URL configurable in devbuilds #30

Conversation

dedecej commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhd commented Apr 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhd commented Jun 30, 2016

dedecej commented Jul 11, 2016

pavel-zdenek left a comment

Choose a reason for hiding this comment

pavel-zdenek Oct 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-zdenek Oct 7, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-zdenek commented Oct 17, 2016

fhd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhd commented Oct 28, 2016

fhd commented Oct 28, 2016

pavel-zdenek Oct 7, 2016 •

edited

pavel-zdenek Oct 7, 2016 •

edited