DPS909 Release 02: My First Open Source Contribution Part 2 (WIP)
***As of 2018/03/25, this is a work in progress. This blog post is showing my progress made thus far, and will be updated once the final fix has been coded.
As a follow-up to the first part of this post, this post will cover all the process I've gone through to solve this bug. I will also include useful links to the bug, resources, and my pull request as well.
1. What bug did I choose?
After investigating the project and the kinds of issues that are filed on GitHub, I've chosen an issue that has to do with the Brave menu and submenu, more specifically with the History tab and some of its submenus that need to be disabled after certain conditions are met.
Here is the link to the GitHub issue;
https://github.com/brave/browser-laptop/issues/2681
Here is the original comment about the bug, how to reproduce it, and the used specs;
So essentially, the bug consists of disabling two submenu options below the History tab of the menubar, only when the browser history has been cleared beforehand.
As of this post, the bug still exists, as I was able to reproduce it.
1. Visit a random internet page:
2. Making the History top menu appear, and selecting "Clear Browsing Data". (press Alt to make top menu appear)
3. After doing this, we can check the History to see if it is empty, just to make sure. So now we close the Youtube tab (after having cleared browsing history in step 2), and we double check History.
4. As you can see, it's empty. Now, to reproduce the bug, we re-open the History top menu and check to see if "Reopen Last Closed Tab" and "Reopen Last Closed Window" are disabled in the submenu;
In the History submenu, we can see the "Recently Closed" option is blurred out/disabled. This is the expected behaviour for "Reopen Last Closed Tab" and "Reopen Last Closed Window", however, as the picture shows, both options are still enabled. Both can be clicked, and neither one of them will do anything.
2. How did I fix it?
First, I followed the instructions to properly set up my environment for debugging. This included many steps, some of which I had trouble with, but overall the installation process was not too difficult. Some of the steps I followed can be found here;
https://github.com/brave/browser-laptop (Look at the README file)
Second, the tools I used to try and analyze the code structure further was Atom and Chrome DevTools.
Atom: This software allowed me to debug the code by modifying the files that needed modifications. This combined with the two PowerShell windows that ran "npm run start" and "npm run watch", it allowed for near-instant refreshing of the modifications made to the files.
Chrome DevTools: As I got further along in debugging, I needed to know some of what the program would pass as values through the multiple layers of architectural code. To do this, I used the Chrome DevTools (Shift+F8), more specifically, the CallStack and Breakpoints in the "Sources" tab.
2.1. How did I approach the problem at first?
As I read the issue post on the GitHub page, (https://github.com/brave/browser-laptop/issues/2681), I found a good first hint from a comment on how to approach the problem;
So the first few files I looked at were;
js/state/frameStateUtil.js
js/actions/WindowActions.js
js/constants/WindowConstants.js
app/renderer/components/main/main.js
js/stores/appStores.js
Some of these files I needed to put console output to figure out the Call Stack or what was bring passed from a function to another. Other files needed added functionality like mentioned in the comment of the issue.
First of all, I wanted to find the portion of the code that handles the opening of the menu, to see where the menu for the History tab opens up. Since this menu opens from the (Alt) keyboard shortcut, I found this portion of code;
File: app/renderer/components/main/main.js
From this I figured out that the processing for the top menu is wrapped here, and any method call related to functionality from this menu is handled by windowActions.
As mentioned in the comment from the issue on GitHub, the actions involved in this issue will most likely be appActions as opposed to windowActions, since the functionality for clearing browsing data, which is a required step to reproduce this bug, is within an appAction label named APP_ON_CLEAR_BROWSING_DATA.
As this piece of code was my starting point towards figuring out the inner workings of the project, I will go from here and write down the modifications i've made along with reasons why.
1. Making a new appConstant variable (appConstants.js)
Every constant variable declared in this file is later invoked from a switch case statement located in the appStore.js file. The first step to implementing the additional logic was to make the label in this file, since we will want to add functionality that takes care of closing unneeded History submenu items when clearing browsing data from the History submenu.
I've added the APP_CLEAR_CLOSED_FRAMES constant here.
2. Finding a dispatching statement in appActions (appActions.js)
I found out the functionality on a window level already existed, in the windowActions.js file from line 200 to 209. The code looked like this:
Since I understand that I need to reproduce this on an appConstant, I've reproduced the code in appActions.js with the newly created constant variable created in step 1;
3. Replicating the similar changes in step 2 but for windowStore.js to appStore.js (appStore.js)
The windowStore.js file takes care of executing the commands that are dispatched from the windowActions.js file. Since we created the equivalent dispatch for appActions.js, we need to create the equivalent code execution for appStore.js. Here are my changes;
***PRE-FIX PICTURE
***NOTE: As of 2018/03/25, this code does not fix. This picture shows the logic I've added for the new appConstant tag I've added above. As of now, I know the closedFrames set from the appState need to refer to the submenu items from the History menubar, but I do not know how to access those.
4. frameStateUtil.js functionality recommended from GitHub user (frameStateUtil.js)
Referencing the suggestion to add a clearClosedFrames method in this file that would set the closedFrames to an empty immutable list, this is the code I've written.
***NOTE: As of 2018/03/25, this code does not fix. It is commented out because I do not yet understand the connection between the appConstant functionality implemented above and the frameStateUtil.js file functionality. Since no apparent parallel functionality exists that I can take as an example, I need more information and/or help to advance further in the project.
5. Adding a call to the APP_CLEAR_CLOSED_FRAMES functionality (menu.js)
Finally, since the action of disabling the buttons in the History submenu should happen after the browser history has been cleared, the logic implies the call should be made after the original call to clearing browser history has been made. Therefore, we can add a call after this has been made in the menu.js file (Line 687 in picture);
2.2. Starting back to square one... (UPDATE 2018/04/03)
As mentioned above, the solution I had come up with did not end up in a fix to this bug. However, I did poke at many places for help on the matter, and one of those responded. Following advice from both my professor David Humphrey and an old classmate of mine, I re-planned my programming completely by changing the way I was looking at the problem. Essentially, I had tunnel-visioned towards this way of fixing the problem, and I stubbornly kept trying once I had met a dead-end. This is the approach with which I was able to solve this problem;
Based on the bug, we can understand that it's about unnatural behaviour, since the reopen last closed tab and window buttons should not be available if no history exists. This got me thinking; "What is the current default behaviour? If I open a Brave browser for the first time, are these buttons enabled?" The answer is yes, which changed the way I looked at the problem.
Step 1: Check where the submenu is initialized in the code;
app/common/commonMenu.js
This part of code is included within the function that is called when the submenu gets re-created. The structure of the module.exports.(function name) that was there prior to my changes only included the label, accelerator and item.click initialization. I've added the if-else loop structure as well as a new variable (isDisabled) to make a structure that could determine whether or not to set the submenu item to 'enabled = false'.
3. What I've learned from the experience
***NOTE: As of 2018/03/25, this code does not fix it. Since this code is incomplete further feedback and help will be needed before this can be considered a final product and a legitimate pull request.
Working from scratch into a complex architectural code structure like this one is quite the challenge. Here I list a couple of things I've learned from working on this project;
- Ask for help early in the process for things you foresee that will cause problems; By this, I'm referring to understanding how a framework communicates within itself as an example. Knowing exactly how each piece communicates with one another is a huge advantage in this type of project environment.
- Use the community of a project, and any devs that make themselves available to request for help; This relates to the previous statement, however I've found out Brave has a presence even in online chat systems like Discord, so I realized the places I could poke for help were wider than I originally thought.
- Analyze similar bugs before making a choice on your own bug; Especially in project structures such as this one, finding anything similar helps a lot to understand what is being asked and what needs to be done. This is especially more useful when you run into bugs with very minimal detail.
That being said, this experience was very valuable in that it exposed me to something I was fairly new at. Regardless of the issues I've encountered or the problems I managed to fix, I can be proud to say I've contributed something to the wide Brave community.
As a follow-up to the first part of this post, this post will cover all the process I've gone through to solve this bug. I will also include useful links to the bug, resources, and my pull request as well.
1. What bug did I choose?
After investigating the project and the kinds of issues that are filed on GitHub, I've chosen an issue that has to do with the Brave menu and submenu, more specifically with the History tab and some of its submenus that need to be disabled after certain conditions are met.
Here is the link to the GitHub issue;
https://github.com/brave/browser-laptop/issues/2681
Here is the original comment about the bug, how to reproduce it, and the used specs;
As of this post, the bug still exists, as I was able to reproduce it.
1. Visit a random internet page:
2. Making the History top menu appear, and selecting "Clear Browsing Data". (press Alt to make top menu appear)
3. After doing this, we can check the History to see if it is empty, just to make sure. So now we close the Youtube tab (after having cleared browsing history in step 2), and we double check History.
4. As you can see, it's empty. Now, to reproduce the bug, we re-open the History top menu and check to see if "Reopen Last Closed Tab" and "Reopen Last Closed Window" are disabled in the submenu;
In the History submenu, we can see the "Recently Closed" option is blurred out/disabled. This is the expected behaviour for "Reopen Last Closed Tab" and "Reopen Last Closed Window", however, as the picture shows, both options are still enabled. Both can be clicked, and neither one of them will do anything.
2. How did I fix it?
First, I followed the instructions to properly set up my environment for debugging. This included many steps, some of which I had trouble with, but overall the installation process was not too difficult. Some of the steps I followed can be found here;
https://github.com/brave/browser-laptop (Look at the README file)
Second, the tools I used to try and analyze the code structure further was Atom and Chrome DevTools.
Chrome DevTools: As I got further along in debugging, I needed to know some of what the program would pass as values through the multiple layers of architectural code. To do this, I used the Chrome DevTools (Shift+F8), more specifically, the CallStack and Breakpoints in the "Sources" tab.
2.1. How did I approach the problem at first?
As I read the issue post on the GitHub page, (https://github.com/brave/browser-laptop/issues/2681), I found a good first hint from a comment on how to approach the problem;
So the first few files I looked at were;
js/state/frameStateUtil.js
js/actions/WindowActions.js
js/constants/WindowConstants.js
app/renderer/components/main/main.js
js/stores/appStores.js
Some of these files I needed to put console output to figure out the Call Stack or what was bring passed from a function to another. Other files needed added functionality like mentioned in the comment of the issue.
First of all, I wanted to find the portion of the code that handles the opening of the menu, to see where the menu for the History tab opens up. Since this menu opens from the (Alt) keyboard shortcut, I found this portion of code;
File: app/renderer/components/main/main.js
From this I figured out that the processing for the top menu is wrapped here, and any method call related to functionality from this menu is handled by windowActions.
As mentioned in the comment from the issue on GitHub, the actions involved in this issue will most likely be appActions as opposed to windowActions, since the functionality for clearing browsing data, which is a required step to reproduce this bug, is within an appAction label named APP_ON_CLEAR_BROWSING_DATA.
As this piece of code was my starting point towards figuring out the inner workings of the project, I will go from here and write down the modifications i've made along with reasons why.
1. Making a new appConstant variable (appConstants.js)
Every constant variable declared in this file is later invoked from a switch case statement located in the appStore.js file. The first step to implementing the additional logic was to make the label in this file, since we will want to add functionality that takes care of closing unneeded History submenu items when clearing browsing data from the History submenu.
I've added the APP_CLEAR_CLOSED_FRAMES constant here.
2. Finding a dispatching statement in appActions (appActions.js)
I found out the functionality on a window level already existed, in the windowActions.js file from line 200 to 209. The code looked like this:
Since I understand that I need to reproduce this on an appConstant, I've reproduced the code in appActions.js with the newly created constant variable created in step 1;
3. Replicating the similar changes in step 2 but for windowStore.js to appStore.js (appStore.js)
The windowStore.js file takes care of executing the commands that are dispatched from the windowActions.js file. Since we created the equivalent dispatch for appActions.js, we need to create the equivalent code execution for appStore.js. Here are my changes;
***PRE-FIX PICTURE
***NOTE: As of 2018/03/25, this code does not fix. This picture shows the logic I've added for the new appConstant tag I've added above. As of now, I know the closedFrames set from the appState need to refer to the submenu items from the History menubar, but I do not know how to access those.
4. frameStateUtil.js functionality recommended from GitHub user (frameStateUtil.js)
Referencing the suggestion to add a clearClosedFrames method in this file that would set the closedFrames to an empty immutable list, this is the code I've written.
***NOTE: As of 2018/03/25, this code does not fix. It is commented out because I do not yet understand the connection between the appConstant functionality implemented above and the frameStateUtil.js file functionality. Since no apparent parallel functionality exists that I can take as an example, I need more information and/or help to advance further in the project.
5. Adding a call to the APP_CLEAR_CLOSED_FRAMES functionality (menu.js)
Finally, since the action of disabling the buttons in the History submenu should happen after the browser history has been cleared, the logic implies the call should be made after the original call to clearing browser history has been made. Therefore, we can add a call after this has been made in the menu.js file (Line 687 in picture);
2.2. Starting back to square one... (UPDATE 2018/04/03)
As mentioned above, the solution I had come up with did not end up in a fix to this bug. However, I did poke at many places for help on the matter, and one of those responded. Following advice from both my professor David Humphrey and an old classmate of mine, I re-planned my programming completely by changing the way I was looking at the problem. Essentially, I had tunnel-visioned towards this way of fixing the problem, and I stubbornly kept trying once I had met a dead-end. This is the approach with which I was able to solve this problem;
Based on the bug, we can understand that it's about unnatural behaviour, since the reopen last closed tab and window buttons should not be available if no history exists. This got me thinking; "What is the current default behaviour? If I open a Brave browser for the first time, are these buttons enabled?" The answer is yes, which changed the way I looked at the problem.
Step 1: Check where the submenu is initialized in the code;
app/common/commonMenu.js
This part of code is included within the function that is called when the submenu gets re-created. The structure of the module.exports.(function name) that was there prior to my changes only included the label, accelerator and item.click initialization. I've added the if-else loop structure as well as a new variable (isDisabled) to make a structure that could determine whether or not to set the submenu item to 'enabled = false'.
3. What I've learned from the experience
***NOTE: As of 2018/03/25, this code does not fix it. Since this code is incomplete further feedback and help will be needed before this can be considered a final product and a legitimate pull request.
Working from scratch into a complex architectural code structure like this one is quite the challenge. Here I list a couple of things I've learned from working on this project;
- Ask for help early in the process for things you foresee that will cause problems; By this, I'm referring to understanding how a framework communicates within itself as an example. Knowing exactly how each piece communicates with one another is a huge advantage in this type of project environment.
- Use the community of a project, and any devs that make themselves available to request for help; This relates to the previous statement, however I've found out Brave has a presence even in online chat systems like Discord, so I realized the places I could poke for help were wider than I originally thought.
- Analyze similar bugs before making a choice on your own bug; Especially in project structures such as this one, finding anything similar helps a lot to understand what is being asked and what needs to be done. This is especially more useful when you run into bugs with very minimal detail.
That being said, this experience was very valuable in that it exposed me to something I was fairly new at. Regardless of the issues I've encountered or the problems I managed to fix, I can be proud to say I've contributed something to the wide Brave community.
Comments
Post a Comment